Re: [PATCH] mm: migrate: Fix THP's mapcount on isolation

[Gavin Shan]

Hi Alistair,

On 11/23/22 12:26 PM, Alistair Popple wrote:
> Gavin Shan <gshan@xxxxxxxxxx> writes:
>
> > The issue is reported when removing memory through virtio_mem device.
> > The transparent huge page, experienced copy-on-write fault, is wrongly
> > regarded as pinned. The transparent huge page is escaped from being
> > isolated in isolate_migratepages_block(). The transparent huge page
> > can't be migrated and the corresponding memory block can't be put
> > into offline state.
> >
> > Fix it by replacing page_mapcount() with total_mapcount(). With this,
> > the transparent huge page can be isolated and migrated, and the memory
> > block can be put into offline state.
> >
> > Fixes: 3917c80280c9 ("thp: change CoW semantics for anon-THP")
> > Cc: stable@xxxxxxxxxxxxxxx   # v5.8+
> > Reported-by: Zhenyu Zhang <zhenyzha@xxxxxxxxxx>
> > Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> > Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
> > ---
> >   mm/compaction.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index c51f7f545afe..c408b5e04c1d 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -990,7 +990,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >   		 * admittedly racy check.
> >   		 */
> >   		mapping = page_mapping(page);
> > -		if (!mapping && page_count(page) > page_mapcount(page))
> > +		if (!mapping && page_count(page) > total_mapcount(page))
>
> We have several versions of these checks for pinned pages open-coded
> around the place. See for example migrate_vma_check_page() and
> folio_expected_refs(). It looks like you could use a variant of
> migrate_vma_check_page() which would also check non-anon pins, although
> I don't know the compaction code well enough to know if that's useful.
>
> Either way it would be nice if we had a common helper for these kind of
> checks. Guess that would be harder to backport, and the change itself
> looks ok. But why isn't the fixes tag 119d6d59dcc0 ("mm, compaction:
> avoid isolating pinned pages")?

Nice to see your comments. folio_expected_refs() only returns the
mapcount for file-mapped pages. migrate_vma_check_page() doesn't
cover THP and there is a 'FIXME' there. A unified helper is beneficial
to maintainance. I think the issue can be fixed in place and have a
followup patch to introduce the unified helper, to make the backporting
happy.

Right, I was thinking of 119d6d59dcc0, which was merged in early days
back to v3.15. I doubt we even had THP support that time. 3917c80280c9
changed the behavior of THP COW handling, to split the THP. Without
3917c80280c9, no splitting is expected and the original condition is
correct to check for anon pins.

> >   			goto isolate_fail;
> >   
> >   		/*

Thanks,
Gavin