Messages in this thread | | | Date | Wed, 23 Nov 2022 09:56:38 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH] mm: migrate: Fix THP's mapcount on isolation |
| |
On 23.11.22 06:14, Hugh Dickins wrote: > On Wed, 23 Nov 2022, 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. >> >> Fixes: 3917c80280c9 ("thp: change CoW semantics for anon-THP") >> Cc: stable@vger.kernel.org # v5.8+ >> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Gavin Shan <gshan@redhat.com> > > Interesting, good catch, looked right to me: except for the Fixes line > and mention of v5.8. That CoW change may have added a case which easily > demonstrates the problem, but it would have been the wrong test on a THP > for long before then - but only in v5.7 were compound pages allowed > through at all to reach that test, so I think it should be > > Fixes: 1da2f328fa64 ("mm,thp,compaction,cma: allow THP migration for CMA allocations") > Cc: stable@vger.kernel.org # v5.7+ > > Oh, no, stop: this is not so easy, even in the latest tree. > > Because at the time of that "admittedly racy check", we have no hold > at all on the page in question: and if it's PageLRU or PageCompound > at one instant, it may be different the next instant. Which leaves it > vulnerable to whatever BUG_ON()s there may be in the total_mapcount() > path - needs research. *Perhaps* there are no more BUG_ON()s in the > total_mapcount() path than in the existing page_mapcount() path. > > I suspect that for this to be safe (before your patch and more so after), > it will be necessary to shift the "admittedly racy check" down after the > get_page_unless_zero() (and check the sequence of operations when a > compound page is initialized).
Grabbing a reference first sounds like the right approach to me.
> > The races I'm talking about are much much rarer than the condition you > are trying to avoid, so it's frustrating; but such races are real, > and increasing stable's exposure to them is not so good.
Such checks are always racy and the code has to be able to deal with false negatives/postives (we're not even holding the page lock); as you state, we just don't want to trigger undefined behavior/BUG.
I'm also curious how that migration code handles a THP that's in the swapcache. It better should handle such pages correctly, for example, by removing them from the swapcache first, otherwise that could block migration.
For example, in mm/ksm.c:write_protect_page() we have
"page_mapcount(page) + 1 + swapped != page_count(page)"
page_mapcount() and "swapped==0/1" makes sense to me, because KSM only cares about order-0 pages, so no need for THP games.
But we do have an even better helper in place already: mm/huge_memory.c:can_split_folio()
Which cares about
a) Swapcache for THP: each subpage could be in the swapcache b) Requires the caller to hold one reference to be safe
But I am a bit confused about the "extra_pins" for !anon. Where do the folio_nr_pages() references come from?
So *maybe* it makes sense to factor out can_split_folio() and call it something like: "folio_maybe_additionally_referenced" [to clearly distinguish it from "folio_maybe_dma_pinned" that cares about actual page pinning (read/write page content)].
Such a function could return false positives/negatives due to races and the caller would have to hold one reference and be able to deal with the semantics.
-- Thanks,
David / dhildenb
| |