Re: [PATCHv7 13/18] mm: Drop fake head checks

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/27/26 13:00, Kiryl Shutsemau wrote:
> On Tue, Oct 27, 2026 at 12:00:28PM +0100, Peter Zijlstra wrote:
>> On Tue, Oct 27, 2026 at 12:37:13AM +0100, David Hildenbrand (Arm) wrote:
>>>
>>> I'm sure x86 folks will have an opinion :) . Let me CC them whether something
>>> like that would be acceptable.
>>
>> This looks quite terrible, doubly so because I have no idea what its
>> trying to fix. Please try and justify this insanity.
> 
> Fair enough. Here's some context.
> 
> HVO (HugeTLB Vmemmap Optimization) frees most of the memmap of a hugetlb
> folio by remapping the struct pages after the first vmemmap page to a
> shared page, mapped read-only.
> 
> Every lockless get of a page reference (e.g. page cache lookup, PFN
> scanners) relies on page_ref_add_unless_zero(), which is
> atomic_add_unless(&page->_refcount, nr, 0).
> 
> The page can be freed and reused between finding it and the get; the
> zero refcount is what makes the get fail. The race:
> 
>   lockless get                        other CPU
>   c = atomic_read(&page->_refcount)
>   c != 0
>                                       page freed
>                                       page allocated as a hugetlb tail
>                                       HVO remaps it read-only
>   try_cmpxchg(&page->_refcount, &c, c + nr)
>     -> write fault on read-only vmemmap
> 
> The refcount is zero by then and the compare fails, but LOCK CMPXCHG
> writes anyway.
> 
> Before v7.1, HVO mapped the tail vmemmap pages to the head vmemmap page
> of the same folio. The first struct page in each of them read as the
> head, with PG_head and the head's refcount: a fake head. A get on a fake
> head of a folio in use always tried the write, not only in a race, so
> page_ref_add_unless() checked for fake heads before the atomic.
> 
> Back then, the race above was closed by bd225530a4c7: rcu_read_lock()
> around the read and the cmpxchg, and synchronize_rcu() in HVO before the
> remap. The refcounts are already zero by then, so the get either
> finishes its cmpxchg before the remap or reads zero and bails.
> 
> Since v7.1, tails encode the head as a mask instead of a pointer, so
> tails of the same order are identical and HVO maps them to one page of
> real tails, shared by the zone. Fake heads are gone, and every read-only
> struct page is a tail with a zero refcount. The race is not gone, but
> 32c440d67e6c and 01b1d0ffb6f7 dropped the RCU on both sides anyway, on
> the wrong belief that it was only there for fake heads. That is what
> David reported.
> 
> One fix is to put the RCU back. It works, but it means rcu_read_lock()
> around every get that goes through page_ref_add_unless_zero(), GUP-fast
> included, and synchronize_rcu() on hugetlb allocation and freeing,
> including surplus allocation at fault time. So I am exploring other
> options.
> 
> The fixup is one of them. With the new layout, a fault on that CMPXCHG
> can only be a write to an HVO tail, so the refcount is zero and the get
> fails, and the fault only happens in the window above. With GCC the loop
> compiles to the same code as before; clang adds a SETE/TEST per call
> site.
> 
> Writing this, I just realized that we have another option -- map the
> shared tail page writable. A failing LOCK CMPXCHG writes back the value
> it read, and the get never changes a zero refcount, so the race becomes
> harmless. The cost is the protection: the page is shared by every
> hugetlb folio of that order in the zone, so a stray write to any tail
> silently corrupts all of them instead of oopsing.

Right. Without HVO we would just corrupt pages in the single folio (which is
also not nice).

I'm also questioning whether we need this RO protection all the time active.

-- 
Cheers,

David