Re: [PATCH v2 0/6] mm/arm64: re-enable HVO

[Will Deacon <will@kernel.org>]

On Mon, Nov 25, 2024 at 03:22:47PM -0700, Yu Zhao wrote:
> On Mon, Nov 25, 2024 at 8:22 AM Will Deacon <will@kernel.org> wrote:
> > On Thu, Nov 07, 2024 at 01:20:27PM -0700, Yu Zhao wrote:
> > > HVO was disabled by commit 060a2c92d1b6 ("arm64: mm: hugetlb: Disable
> > > HUGETLB_PAGE_OPTIMIZE_VMEMMAP") due to the following reason:
> > >
> > >   This is deemed UNPREDICTABLE by the Arm architecture without a
> > >   break-before-make sequence (make the PTE invalid, TLBI, write the
> > >   new valid PTE). However, such sequence is not possible since the
> > >   vmemmap may be concurrently accessed by the kernel.
> > >
> > > This series presents one of the previously discussed approaches to
> > > re-enable HugeTLB Vmemmap Optimization (HVO) on arm64.
> >
> > Before jumping into the new mechanisms here, I'd really like to
> > understand how the current code is intended to work in the relatively
> > simple case where the vmemmap is page-mapped to start with (i.e. when we
> > don't need to worry about block-splitting).
> >
> > In that case, who are the concurrent users of the vmemmap that we need
> > to worry about?
> 
> Any speculative PFN walkers who either only read `struct page[]` or
> attempt to increment page->_refcount if it's not zero.
> 
> > Is it solely speculative references via
> > page_ref_add_unless() or are there others?
> 
> page_ref_add_unless() needs to be successful before writes can follow;
> speculative reads are always allowed.
> 
> > Looking at page_ref_add_unless(), what serialises that against
> > __hugetlb_vmemmap_restore_folio()? I see there's a synchronize_rcu()
> > call in the latter, but what prevents an RCU reader coming in
> > immediately after that?
> 
> In page_ref_add_unless(), the condtion `!page_is_fake_head(page) &&
> page_ref_count(page)` returns false before a PTE becomes RO.
> 
> For HVO, i.e., a PTE being switched from RW to RO, page_ref_count() is
> frozen (remains zero), followed by synchronize_rcu(). After the
> switch, page_is_fake_head() is true and it appears before
> page_ref_count() is unfrozen (become non-zero), so the condition
> remains false.
> 
> For de-HVO, i.e., a PTE being switched from RO to RW, page_ref_count()
> again is frozen, followed by synchronize_rcu(). Only this time
> page_is_fake_head() is false after the switch, and again it appears
> before page_ref_count() is unfrozen. To answer your question, readers
> coming in immediately after that won't be able to see non-zero
> page_ref_count() before it sees page_is_fake_head() being false. IOW,
> regarding whether it is RW, the condition can be false negative but
> never false positive.

Thanks, but I'm still not seeing how this works. When you say "appears
before", I don't see any memory barriers in page_ref_add_unless() that
enforce that e.g. the refcount and the flags are checked in order and
I can't see how the synchronize_rcu() helps either as it's called really
earlyi (I think that's just there for the static key).

If page_is_fake_head() is reliable, then I'm thinking we could use that
to steer page_ref_add_unless() away from the tail pages during the
remapping operations and it would be fine to use a break-before-make
sequence.

Will