On 28.06.22 01:37, Nadav Amit wrote: > [ +Dave Hansen to say how wrong I am ] > >> On Jun 27, 2022, at 6:12 AM, Peter Xu <peterx@xxxxxxxxxx> wrote: >> >> ⚠ External Email >> >> On Sat, Jun 25, 2022 at 07:49:54AM +0000, Nadav Amit wrote: >>> >>> >>>> On Jun 24, 2022, at 3:17 PM, Peter Xu <peterx@xxxxxxxxxx> 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. >> >> I'd hope we don't make an interface design just to service that purpose of >> when write=0 and dirty=1 use case that is internal to the kernel so far, >> and I still think it's the tlb flush code to change.. or do we have other >> use case for this WRITE_LIKELY hint? >> >> For UFFDIO_CONTINUE, if we want to make things clear on dirty bit, then >> IMHO for UFFDIO_CONTINUE the right place for the dirty process is where the >> user writes to the page in the other mapping, where PageDirty() will start >> to be true already even if the pte that to be CONTINUEd will have dirty=0 >> in the pte entry. From that pov I still don't see why we need to grant the >> user on the dirty bit control, no matter with a hint only, or explicit. >> >>> >>> 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!) >> >> David convinced you I think :) >> >>> 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). >> >> The hint idea sounds good to me, it's just that we actually have two steps >> here: >> >> (1) We think providing user the control of dirty bit makes sense, then, >> (2) We think the flag should be a hint not explicit "set dirty bit" >> >> I agree with (2) in this case if (1) is applicable. And now I think I'm >> questioning myself on (1). >> >> Fundamentally, access bit has more meaningful context (0 means cold, 1 >> means hot), for dirty it's really more a perf thing to me (when clear, >> it'll take extra cycles to set it when memory write happens to it; being >> clear _may_ help only for the tlb flush example you mentioned but I'm not >> fully convinced that's correct). > > I am not sure we understand each other. I think the benefit of not setting > a dirty-bit when a page is not actually written is fundamental, and has > inherit performance benefit. > > When I did x86’s pte_flags_need_flush(), I was defensive, but there is a > basic optimization that is possible to avoid a TLB flush on non-dirty > writable PTEs. > > In x86, consider a situation in which you use ptep_modify_prot_start() > to remove a PTE and load its old value using xchg. (A similar case happens > on reclaim). Assume you want to write-protect the entry. > > If the PTE is non-dirty then you should be able to avoid a flush, even if > the PTE is writable. In x86, a write and the change of the dirty-bit are > performed both atomically. Therefore, if the dirty-bit on the old PTE was > clear, you can avoid a TLB flush. > > Besides the benefit of avoiding a TLB flush, there is also the benefit > of having more precise dirty tracking. You assume UFFDIO_CONTINUE will be > preceded by memory write to the shared memory, but that does not have to > be the case. Similarly, if in the future userfaultfd would also support > memory-backed private mappings, that does not have to be the case either. > > Putting all of the above aside, there is a bug in my code, but this > bug also points why dirty should not be set unconditionally. If someone > uses SOFT_DIRTY with userfaultfd, then marking the PTE as dirty (and > soft-dirty) might be misleading, causing unnecessary userspace writeback > of memory. > > So I do need to fix my code so it would not write-unprotect memory if > soft-dirty is enabled and UFFD_FLAGS_WRITE_LIKELY is not provided. But > I think it emphasizes the benefit of having UFFD_FLAGS_WRITE_LIKELY. > >> >> Maybe with the to be proposed RFC patch for tlb flush we can know whether >> that should be something we can rely on. It'll add more dependency on this >> work which I'm sorry to say. It's just that IMHO we should think carefully >> for the write-hint because this is a solid new uABI we're talking about. >> >> The other option is we can introduce the access hint first and think more >> on the dirty one (we can always add it when proper). What do you think? >> Also, David please chim in anytime if I missed the whole point when you >> proposed the idea. >> >>> >>> 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 . >> >> I'm not sure I'm right here, but I think David's patch should have covered >> that case? The new helper only checks pte_uffd_wp() based on my memory, >> and when resolving page faults uffd-wp bit should have been gone, so it >> should be treated the same as normal ptes. > > Let’s see we get to the same page: > > mwriteprotect_range() does: > > change_protection(&tlb, dst_vma, start, start + len, newprot, > enable_wp ? MM_CP_UFFD_WP : MM_CP_UFFD_WP_RESOLVE) > > As you see no use of MM_CP_TRY_CHANGE_WRITABLE. > > And then change_pte_range() does: > > if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) && > !pte_write(ptent) && > can_change_pte_writable(vma, addr, ptent)) > ptent = pte_mkwrite(ptent); Right, I think in a previous version of my patch (before you guys convinced me to introduce MM_CP_TRY_CHANGE_WRITABLE :P ) it would have done it automatically (for private mappings). We might have to add it to some callers now manually to not only consider mprotect. -- Thanks, David / dhildenb
