Re: [RFC PATCH v1 0/4] Reduce cost of ptep_get_lockless on arm64

[David Hildenbrand]

On 15.04.24 11:28, Ryan Roberts wrote:
> On 12/04/2024 21:16, David Hildenbrand wrote:
> > > Yes agreed - 2 types; "lockless walkers that later recheck under PTL" and
> > > "lockless walkers that never take the PTL".
> > >
> > > Detail: the part about disabling interrupts and TLB flush syncing is
> > > arch-specifc. That's not how arm64 does it (the hw broadcasts the TLBIs). But
> > > you make that clear further down.
> >
> > Yes, but disabling interrupts is also required for RCU-freeing of page tables
> > such that they can be walked safely. The TLB flush IPI is arch-specific and
> > indeed to sync against PTE invalidation (before generic GUP-fast).
> > [...]
> >
> > > > > Could it be this easy? My head is hurting...
> > > >
> > > > I think what has to happen is:
> > > >
> > > > (1) pte_get_lockless() must return the same value as ptep_get() as long as there
> > > > are no races. No removal/addition of access/dirty bits etc.
> > >
> > > Today's arm64 ptep_get() guarantees this.
> > >
> > > > (2) Lockless page table walkers that later verify under the PTL can handle
> > > > serious "garbage PTEs". This is our page fault handler.
> > >
> > > This isn't really a property of a ptep_get_lockless(); its a statement about a
> > > class of users. I agree with the statement.
> >
> > Yes. That's a requirement for the user of ptep_get_lockless(), such as page
> > fault handlers. Well, mostly "not GUP".
> >
> > > > (3) Lockless page table walkers that cannot verify under PTL cannot handle
> > > > arbitrary garbage PTEs. This is GUP-fast. Two options:
> > > >
> > > > (3a) pte_get_lockless() can atomically read the PTE: We re-check later if the
> > > > atomically-read PTE is still unchanged (without PTL). No IPI for TLB flushes
> > > > required. This is the common case. HW might concurrently set access/dirty bits,
> > > > so we can race with that. But we don't read garbage.
> > >
> > > Today's arm64 ptep_get() cannot garantee that the access/dirty bits are
> > > consistent for contpte ptes. That's the bit that complicates the current
> > > ptep_get_lockless() implementation.
> > >
> > > But the point I was trying to make is that GUP-fast does not actually care about
> > > *all* the fields being consistent (e.g. access/dirty). So we could spec
> > > pte_get_lockless() to say that "all fields in the returned pte are guarranteed
> > > to be self-consistent except for access and dirty information, which may be
> > > inconsistent if a racing modification occured".
> >
> > We *might* have KVM in the future want to check that a PTE is dirty, such that
> > we can only allow dirty PTEs to be writable in a secondary MMU. That's not there
> > yet, but one thing I was discussing on the list recently. Burried in:
> >
> > https://lkml.kernel.org/r/20240320005024.3216282-1-seanjc@xxxxxxxxxx
> >
> > We wouldn't care about racing modifications, as long as MMU notifiers will
> > properly notify us when the PTE would lose its dirty bits.
> >
> > But getting false-positive dirty bits would be problematic.
> >
> > > This could mean that the access/dirty state *does* change for a given page while
> > > GUP-fast is walking it, but GUP-fast *doesn't* detect that change. I *think*
> > > that failing to detect this is benign.
> >
> > I mean, HW could just set the dirty/access bit immediately after the check. So
> > if HW concurrently sets the bit and we don't observe that change when we
> > recheck, I think that would be perfectly fine.
>
> Yes indeed; that's my point - GUP-fast doesn't care about access/dirty (or
> soft-dirty or uffd-wp).
>
> But if you don't want to change the ptep_get_lockless() spec to explicitly allow
> this (because you have the KVM use case where false-positive dirty is
> problematic), then I think we are stuck with ptep_get_lockless() as implemented
> for arm64 today.

At least regarding the dirty bit, we'd have to guarantee that if 
ptep_get_lockless() returns a false-positive dirty bit, that the PTE 
recheck would be able to catch that.

Would that be possible?

--
Cheers,

David / dhildenb