[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c61612f7-b861-39cf-3e73-dbe4d134eec0@redhat.com>
Date: Wed, 23 Nov 2022 09:56:38 +0100
From: David Hildenbrand <david@...hat.com>
To: Hugh Dickins <hughd@...gle.com>, Gavin Shan <gshan@...hat.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
akpm@...ux-foundation.org, william.kucharski@...cle.com,
ziy@...dia.com, kirill.shutemov@...ux.intel.com,
zhenyzha@...hat.com, shan.gavin@...il.com, riel@...riel.com,
willy@...radead.org, apopple@...dia.com
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@...r.kernel.org # v5.8+
>> Reported-by: Zhenyu Zhang <zhenyzha@...hat.com>
>> Suggested-by: David Hildenbrand <david@...hat.com>
>> Signed-off-by: Gavin Shan <gshan@...hat.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@...r.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
Powered by blists - more mailing lists