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

[Nadav Amit <namit@vmware.com>]

> On Jun 24, 2022, at 3:17 PM, Peter Xu <peterx@redhat.com> wrote:
> 
> On Fri, Jun 24, 2022 at 05:58:17PM -0400, Peter Xu wrote:
>> [Sorry for replying late]
>> 
>> 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?
> 
> Hmm, hold on...  I failed to figure out how that write-likely hint could
> help us for either huge or non-huge pages, since:
> 
>  (1) Old code always set dirty, so no perf degrade anyway with/without the
>      hint
> 
>  (2) If we want to rework dirty bit (which I'm totally fine with..), then
>      we don't apply it when we shouldn't, and afaict we should set D bit
>      whenever we should...  if the user assumes this page is likely to be
>      written but made it read-only, say, with UFFDIO_COPY(wp_mode=1),
>      setting D bit will not help, instead, the user should simply use an
>      UFFDIO_COPY(wp_mode=0) then the dirty will be set with write=1..
> 
> It'll be helpful but only helpful for UFFDIO_ZEROCOPY because it avoids one
> COW.  But that seems to be it.
> 
> In short: I'm wondering whether we only really need the ACCESS_LIKELY hint
> as you proposed earlier.  We may want UFFDIO_ZEROPAGE_MODE_ALLOCATE
> separately, but keep that only for zeropage op (and it shouldn't really be
> called WRITE_LIKELY)?  Or did I miss something?

Let’s see if I get you correctly. I am not sure whether we had this
discussion before.

We are talking about a scenario in which WP=0. You argue that if the page
is already set as dirty, what is the benefit of not setting the dirty-bit,
right?

So first, IIUC, there are cases in which the page would not be set as
dirty, e.g., UFFDIO_CONTINUE. [ I am admittedly not too familiar with this
use-case, so I say it based on the comments. ]

Second, even if the page is dirty (e.g., following UFFDIO_COPY), but it
is not written by the user after UFFDI_COPY, marking the PTE as dirty
when it is mapped would induce overhead, as we discussed before, since
if/when the PTE is unmapped, TLB flush batching might not be possible.

So I don’t think there is a problem in having WRITE_LIKELY hint. Moreover,
I would reiterate my position (which you guys convinced me in!) that
having hints that indicate what the user does (WRITE_LIKELY) is a better
API than something that indicates directly what the kernel should do
(e.g., UFFDIO_ZEROPAGE_MODE_ALLOCATE).


But this discussion made me think that there are two somewhat related
matters that we may want to address:

1. mwriteprotect_range() should use MM_CP_TRY_CHANGE_WRITABLE when !wp
to proactively make entries writable and save .

2. The WRITE_LIKELY hint should be propagated from mwriteprotect_range()
to change_pte_range() through cp_flags, and the entry should be set
dirty accordingly.

With that, and with leaving hugetlbfs as it is (I meant before - leaving
it as it is in the patchset that I sent), would it be ok with you?