On Thu, Nov 28, 2024 at 7:20 AM Will Deacon [off-list ref] wrote:

On Mon, Nov 25, 2024 at 03:22:47PM -0700, Yu Zhao wrote:

quoted

On Mon, Nov 25, 2024 at 8:22 AM Will Deacon [off-list ref] wrote:

quoted

On Thu, Nov 07, 2024 at 01:20:27PM -0700, Yu Zhao wrote:

quoted

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.

quoted

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.

quoted

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

Right, there is a missing barrier in page_ref_add_unless() and the
order of those two checks, i.e., page_is_fake_head() and then
page_ref_count() is wrong.

I posted a fix here [1].

[1] https://lore.kernel.org/20250107043505.351925-1-yuzhao@google.com/ (local)

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).

That fix makes sure no speculative PFN walkers will try to modify
page->_refcount during the transition from the counter being frozen to
modifiable. synchronize_rcu() makes sure something similar won't
happen during the transition from the counter being modifiable to
frozen.

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.

The struct page pointer passed into page_is_fake_head() would become
inaccessible during BBM. So it would just crash there. That's why I
think we either have to handle kernel PFs or pause other CPUs.

(page_is_fake_head() works by detecting whether it's accessing the
original struct page or a remapped (r/o) one, and the latter has a
signature for it to tell.)