Messages in this thread | | | Date | Thu, 24 Nov 2022 11:09:39 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH v2] mm: migrate: Fix THP's mapcount on isolation |
| |
On 24.11.22 10:55, Gavin Shan wrote: > 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. Besides, The page's refcount is > increased a bit earlier to avoid the page is released when the check > is executed.
Did you look into handling pages that are in the swapcache case as well?
See is_refcount_suitable() in mm/khugepaged.c.
Should be easy to reproduce, let me know if you need inspiration.
> > Fixes: 1da2f328fa64 ("mm,thp,compaction,cma: allow THP migration for CMA allocations") > Cc: stable@vger.kernel.org # v5.7+ > Reported-by: Zhenyu Zhang <zhenyzha@redhat.com> > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > v2: Corrected fix tag and increase page's refcount before the check > --- > mm/compaction.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index c51f7f545afe..1f6da31dd9a5 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -984,29 +984,29 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, > goto isolate_fail; > } > > + /* > + * Be careful not to clear PageLRU until after we're > + * sure the page is not being freed elsewhere -- the > + * page release code relies on it. > + */ > + if (unlikely(!get_page_unless_zero(page))) > + goto isolate_fail; > + > /* > * Migration will fail if an anonymous page is pinned in memory, > * so avoid taking lru_lock and isolating it unnecessarily in an > * admittedly racy check. > */ > mapping = page_mapping(page); > - if (!mapping && page_count(page) > page_mapcount(page)) > - goto isolate_fail; > + if (!mapping && (page_count(page) - 1) > total_mapcount(page)) > + goto isolate_fail_put; > > /* > * Only allow to migrate anonymous pages in GFP_NOFS context > * because those do not depend on fs locks. > */ > if (!(cc->gfp_mask & __GFP_FS) && mapping) > - goto isolate_fail; > - > - /* > - * Be careful not to clear PageLRU until after we're > - * sure the page is not being freed elsewhere -- the > - * page release code relies on it. > - */ > - if (unlikely(!get_page_unless_zero(page))) > - goto isolate_fail; > + goto isolate_fail_put; > > /* Only take pages on LRU: a check now makes later tests safe */ > if (!PageLRU(page))
-- Thanks,
David / dhildenb
| |