Re: [PATCH v1 2/5] userfaultfd: introduce access-likely mode for common operations

[Peter Xu <peterx@redhat.com>]

[Sorry for replying late]

On Fri, Jun 24, 2022 at 02:42:21AM +0000, Nadav Amit wrote:
> 
> 
> > On Jun 23, 2022, at 7:05 PM, Peter Xu <peterx@redhat.com> wrote:
> > 
> > On Fri, Jun 24, 2022 at 12:03:38AM +0000, Nadav Amit wrote:
> >> My take is that hints are hints. Following David’s (or was it yours?)
> >> feedback, I fixed the description to indicate that this is merely a hint and
> >> removed all references to dirty/access bits. The kernel therefore can ignore
> >> the hint when it wants to or use it in any other way. I fully agree that
> >> this gives the kernel the ability to change the behavior as needed.
> >> 
> >> Note that for write-protected 4KB zero-page (where we share the zero-page)
> >> we always set the access-bit, regardless of the hint, because it makes
> >> sense: the zero-page is not swappable and therefore the access-bit is set.
> > 
> > The zero-page example makes sense, and yeah that makes the hugetlb behavior
> > making more sense too.
> > 
> >> 
> >> I think that the lesser user-facing documentation there is on how the
> >> feature is *exactly* used by the kernel - is better from an API point of
> >> view.
> >> 
> >> So I see no reason to fail or be forced not to set a page as young, just
> >> because a hint was *not* provided. This would even be a regression in the
> >> behavior. The hint is actually always respected right now, it is just that
> >> even if you do not provide the hint, the access/dirty is set.
> >> 
> >> The only consistency I think worth thinking about is with the dirty-bit, and
> >> I can add it if you want. Note that the access-bit (in x86) might be set
> >> speculatively in contrast to the dirty-bit is only set atomically with a
> >> real access. That’s the reason I think it may make sense not to set the
> >> dirty without a hint.
> > 
> > Sorry to ask if this is (another) naive question: any link/help to explain
> > the speculative behavior on access bit? Is it part of speculative
> > execution (which, iiuc, would it be reverted if the speculation failed)?
> 
> Oh man, it is hard to find a reference. I made this claim it based on my
> recollection (and logic).
> 
> The access-bit on Intel is set when the PTE is loaded into the TLB, so if you
> allow speculative loading of the TLB, that’s what you get.
> 
> Googling shows Yu Zhao saying: "IIRC, there are also false positives, i.e.,
> the accessed bit is set on entries used by speculative execution only.” [1]
> 
> Intel SDM says: "Whenever the processor uses a paging-structure entry as part
> of linear-address translation, it sets the accessed flag in that entry...
> Whenever there is a write to a linear address, the processor sets the dirty
> flag (if it is not already set) in the paging- structure entry..."
> 
> You can argue that this indicates that the access-bit is updated
> speculatively (translations can be speculative) and dirty-bit is on actual
> write. But it is somewhat of a creative reading.
> 
> Googling further did not help much, but I found a relevant discussion on
> RISC-V, in which they actually consider a similar behavior. [2]
> 
> If you want (and care), we can cc Dave Hansen to get a clear answer.
> 
> [1] https://lore.kernel.org/lkml/YE7Rk%2FYA1Uj7yFn2@google.com/
> [2] https://lists.riscv.org/g/tech-virt-mem/topic/accessed_bit/77699883?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,1,80,77699883

I thought even writes can be speculatively executed too?  Though I think
when the speculation was proved wrong the write needs to be reverted along
with making sure D bit cleared if it was cleared before the speculative
operation.

So I think I get you if you meant the access bit may not be reverted even
if we hit a speculative failure (though without solid proofs, afaict..).
IOW we could have false positive access bits set even if not accessed, but
not to D bits which should be accurate.

> 
> > 
> >> 
> >> Is that acceptable? Access-bit always set, dirty-bit according to hint?
> > 
> > I'm still trying to digest what you said above, sorry.
> > 
> > Aren't both access and dirty bits need an atomic op to be set anyway? Then
> > from perf pov should we simply keep setting them both too like what you did
> > with this version? because it seems that'll always avoid an extra pgtable
> > update access?
> 
> I guess by atomic-op you mean atomic-update by the hardware AD-assist.

Yes.

Btw, since I looked at the SDM as you quoted I think that may not strictly
be like an atomic op from processor pov, I guess, since there's a NOTE:

  The accesses used by the processor to set these flags may or may not be
  exposed to the processor’s self-modifying code detection logic. If the
  processor is executing code from the same memory area that is being used
  for the paging structures, the setting of these flags may or may not
  result in an immediate change to the executing code stream.

So I read it as: even if it'll be an atomic, the op can be postponed.

> 
> I agree that if a page is written, the bits would need to be updated and
> these would introduce an overhead. However, if the page cannot be written,
> well, the dirty bit would never be set.

Ok I see what you mean now.  But honestly, I don't think it's anything
related to the speculative access bit behavior described above.. or is it?

> 
> hugetlb_mcopy_atomic_pte() currently does the following:
> 
>         _dst_pte = huge_pte_mkdirty(_dst_pte);
>         _dst_pte = pte_mkyoung(_dst_pte);
> 
>         if (wp_copy)
>                 _dst_pte = huge_pte_mkuffd_wp(_dst_pte);
> 
> Since you asked to update hugetlb_mcopy_atomic_pte(), I can offer three
> options:
> 
> 1. Do not set dirty if (wp_copy).
> 2. Do not set dirty if (wp_copy || !write_hint) 
> 3. Keep it as is.

AFAICT you already go somewhere at least not (3) with non-hugetlb pages in
current series.. because dirty bit is not always set already for them, so
I'd say we'd make them match?  Hugetlbfs shouldn't be special in this
aspect, IMHO.

Said that, I think it doesn't really necessary need to be that complex,
since make_huge_pte() already sets dirty bit when "writable=1", so IIUC
what you need to do is simply make sure dirty bit set when write_hint=1.

Does it sounds correct to you?

-- 
Peter Xu