userfaultfd: support access/write hints

fs/userfaultfd.c +0 -0

 		mmput(ctx->mm);
 	} else {
 		return -ESRCH;

}

ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,

mm/userfaultfd.c +2 -1

-		       unsigned long len, atomic_t *mmap_changing)
+		       unsigned long len, atomic_t *mmap_changing,
+		       uffd_flags_t uffd_flags)
 {
 	return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_CONTINUE,
 			      mmap_changing, 0);
 }

fs/userfaultfd.c static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg) · line 1905

Quoted 6 lines · Nadav Amit

fs/userfaultfd.c +1 -1

@@ -1891,7 +1902,7 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	if (mmget_not_zero(ctx->mm)) {
 		ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,
 				     uffdio_continue.range.len,
-				     &ctx->mmap_changing);
+				     &ctx->mmap_changing, 0);

Shall we consistently use either 0 or UFFD_FLAGS_NONE? I'd go for 0 directly since that's clearer on having "nothing" as flag.

Quoted 3 lines

fs/userfaultfd.c +0 -0

 mmput(ctx->mm);
 	} else {
 		return -ESRCH;

[...]

Quoted 7 lines

ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,

mm/userfaultfd.c +2 -1

-		       unsigned long len, atomic_t *mmap_changing)
+		       unsigned long len, atomic_t *mmap_changing,
+		       uffd_flags_t uffd_flags)

{
	return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_ZEROPAGE,
			      mmap_changing, 0);

I think you agreed on passing the uffd_flags into __mcopy_atomic() (and also below). Is it forgotten or plan changed?

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

Cc Mike Kravetz <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Axel Rasmussen <[email protected]>

Cc Peter Xu <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc Mike Rapoport <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

⚠ External Email

Quoted 18 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

Quoted 8 lines

Cc Mike Kravetz <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Axel Rasmussen <[email protected]>

Cc Peter Xu <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc Mike Rapoport <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

Hmm.. is the hugetlb code overlooked (for both of the hints), or maybe I missed it? Do we need to cover them too? Thanks,

Quoted 30 lines · Peter Xu

⚠ External Email

Quoted 18 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

Quoted 8 lines

Cc Mike Kravetz <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Axel Rasmussen <[email protected]>

Cc Peter Xu <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc Mike Rapoport <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

Quoted 2 lines

Hmm.. is the hugetlb code overlooked (for both of the hints), or maybe I missed it? Do we need to cover them too? Thanks,

I do not know what the value of not setting the PTE’s access/dirty when it comes to performance. The pages won’t be swapped out, just as you wrote in your comment in hugetlb_mcopy_atomic_pte():

/*
* Always mark UFFDIO_COPY page dirty; note that this may not be
* extremely important for hugetlbfs for now since swapping is not
* supported, but we should still be clear in that this page cannot be
* thrown away at will, even if write bit not set.
*/
_dst_pte = huge_pte_mkdirty(_dst_pte);
_dst_pte = pte_mkyoung(_dst_pte);

If you want for consistency/robustness not to set dirty on read-only entries, that’s something that I can do.

Quoted 48 lines · Nadav Amit

Quoted 30 lines · Peter Xu

⚠ External Email

Quoted 18 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

Quoted 8 lines

Cc Mike Kravetz <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Axel Rasmussen <[email protected]>

Cc Peter Xu <[email protected]>

Cc David Hildenbrand <[email protected]>

Cc Mike Rapoport <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

Quoted 2 lines

Hmm.. is the hugetlb code overlooked (for both of the hints), or maybe I missed it? Do we need to cover them too? Thanks,

I do not know what the value of not setting the PTE’s access/dirty when it comes to performance. The pages won’t be swapped out, just as you wrote in your comment in hugetlb_mcopy_atomic_pte():

/*
* Always mark UFFDIO_COPY page dirty; note that this may not be
* extremely important for hugetlbfs for now since swapping is not
* supported, but we should still be clear in that this page cannot be
* thrown away at will, even if write bit not set.
*/
_dst_pte = huge_pte_mkdirty(_dst_pte);
_dst_pte = pte_mkyoung(_dst_pte);

Quoted 2 lines

If you want for consistency/robustness not to set dirty on read-only entries, that’s something that I can do.

We have more than one options here I guess.

We could have the hints not available for hugetlbfs, but then we'll both need to add special document for hugetlbfs (when you write the man-page, let's say) and also it'll be probably better to fail the ioctls with hints when applying to hugetlb vmas.

Leaving them silent to hugetlbfs is another option, it just sounds weird to me without any kind of documentation so I like this one least.

Or we could make them work for hugetlbfs too. Not saying that swap will work some day (but it still could?!) it just seems all consistent as you said.

So I'd prefer the last one, but only if you agree.

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.

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.

Is that acceptable? Access-bit always set, dirty-bit according to hint?

Quoted 9 lines · Nadav Amit

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.

Quoted 10 lines

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.

Quoted 5 lines

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

Quoted 3 lines

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?

Quoted 34 lines · Peter Xu

Quoted 9 lines · Nadav Amit

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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%[email protected]/
[2] https://lists.riscv.org/g/tech-virt-mem/topic/accessed_bit/77699883?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,1,80,77699883

Quoted 5 lines

Quoted 4 lines

Is that acceptable? Access-bit always set, dirty-bit according to hint?

Quoted 6 lines

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.

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.

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.

[Sorry for replying late]

Quoted 56 lines · Nadav Amit

Quoted 34 lines · Peter Xu

Quoted 9 lines · Nadav Amit

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.

Quoted 10 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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%[email protected]/
[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.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Is that acceptable? Access-bit always set, dirty-bit according to hint?

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 9 lines

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

Quoted 6 lines

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?

Quoted 7 lines · Peter Xu

[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?

Quoted 26 lines · Peter Xu

Quoted 7 lines · Peter Xu

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

Quoted 7 lines

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.

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

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.

Quoted 11 lines

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

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.

Quoted 4 lines

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.

Well, if we have an ABI that places pages without further information *why* we're doing that makes us having to guess what to do or what not to do, and I think the zeropage placement is a prime example for that. Personally, I think communicating the intention in forms of hints is something that doesn't leak implementation details into an ABI.

So "no planned access" vs. "read_likely" vs. "write_likely" conceptually makes sense to me.

As I raised previously, *if* we want to let the user affect the dirty bit setting (1) is then a pure implementation detail. Or whatever else we might want to do.

But I also want to raise awareness that architectures that don't have a hw-set dirty bit have to use page faults to mimic dirty tracking. IIRC, s390x is a prime example for that: pte_mkclean() sets the WP bit and marks the page dirty from the write fault. So it's even more expensive than on other architectures.

⚠ External Email

Quoted 41 lines · Nadav Amit

Quoted 26 lines · Peter Xu

Quoted 7 lines · Peter Xu

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

Quoted 7 lines

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?

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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

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.

Quoted 4 lines

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

Quoted 2 lines

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.

[ +Dave Hansen to say how wrong I am ]

Quoted 83 lines · Peter Xu

⚠ External Email

Quoted 41 lines · Nadav Amit

Quoted 26 lines · Peter Xu

Quoted 7 lines · Peter Xu

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

Quoted 7 lines

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?

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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"

Quoted 8 lines

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.

Quoted 15 lines

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.

Quoted 4 lines

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

Quoted 7 lines

Quoted 2 lines

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

[ +Dave Hansen to say how wrong I am ]

Quoted 83 lines · Peter Xu

⚠ External Email

Quoted 41 lines · Nadav Amit

Quoted 26 lines · Peter Xu

Quoted 7 lines · Peter Xu

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

Quoted 7 lines

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?

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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"

Quoted 8 lines

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.

Quoted 16 lines

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.

Quoted 4 lines

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

Quoted 7 lines

Quoted 2 lines

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

If we do not provide MM_CP_TRY_CHANGE_WRITABLE, the PTE would not be writable.

Now for the record, we may want an additional optimization, so if a fault handler is woken because a PTE become writable, we do not retry the page fault (since the PTE is already writable). It is a small change, but let’s see first we agree on the patches so far…

⚠ External Email

Quoted 6 lines · Nadav Amit

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.

Could I ask what's the not supported private mapping you're talking about (and I think you meant "file-backed")? I thought all kinds of private mappings are supported on all modes already?

Thanks for explaining, so yes either false positive or false negative on pte dirty bit can bring a side effect on things, and the user knows the best. Makes sense. I think what I overlooked is the downside of having both write==dirty==1 when it doesn't need to.

Quoted 6 lines

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.

Well I never thought anyone would be using soft-dirty with uffd because logically uffd-wp was somehow trying to replace it with a better interface, hopefully.

If to talk about it, IMHO the most important thing is really not the dirty bit but the write bit: even if you leave both the dirty bits cleared (so the user specified !WRITE_HINT then we grant write bit only), it means when the write happens for sure dirty bit will be set on demand but not soft-dirty since we won't generate a page fault at all. AFAICT it'll be a false negative.

The old code is safe in that respect on that we always set dirty so we can only have false positive (which is acceptable in this case) but not false negative (which is not).

Quoted 5 lines

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.

Sorry to say that, but to me it seems a proof that dirty bit is even more tricky than access bit so we need to have better thoughts, even if the WRITE_LIKELY hint sounds straightforward. I now can buy in all the tlb flush points you explained, but still IMHO that's too fine granule (some random ptes in a batched tlb range flush may not even matter!) and I'm just naively wondering whether we need more thoughts for an uABI to be proposed like that. I won't ask for some use cases and especially numbers to prove assuming I'm asking too much, but really that'll be very persuasive if we can get. I'd not worry for ACCESS_LIKELY on it because that's much more straightforward to me.

Quoted 41 lines

Quoted 17 lines

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.

Quoted 4 lines

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

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

If we do not provide MM_CP_TRY_CHANGE_WRITABLE, the PTE would not be writable.

Now for the record, we may want an additional optimization, so if a fault handler is woken because a PTE become writable, we do not retry the page fault (since the PTE is already writable). It is a small change, but let’s see first we agree on the patches so far…

Ah I see what I missed, thanks.

Another option is we call can_change_pte_writable() somehow in the unprotect branch.

Quoted 14 lines · Peter Xu

⚠ External Email

Quoted 6 lines · Nadav Amit

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.

Quoted 3 lines

Could I ask what's the not supported private mapping you're talking about (and I think you meant "file-backed")? I thought all kinds of private mappings are supported on all modes already?

Yes, I meant file-backed. See vma_can_userfault() for the supported types of memory:

  static inline bool vma_can_userfault(struct vm_area_struct *vma,
                                     unsigned long vm_flags)
  {       
        if (vm_flags & VM_UFFD_MINOR)
                return is_vm_hugetlb_page(vma) || vma_is_shmem(vma);

#ifndef CONFIG_PTE_MARKER_UFFD_WP
        /*
         * If user requested uffd-wp but not enabled pte markers for
         * uffd-wp, then shmem & hugetlbfs are not supported but only
         * anonymous.
         */             
        if ((vm_flags & VM_UFFD_WP) && !vma_is_anonymous(vma))
                return false;
#endif
        return vma_is_anonymous(vma) || is_vm_hugetlb_page(vma) ||
            vma_is_shmem(vma);

}

Quoted 3 lines

Well I never thought anyone would be using soft-dirty with uffd because logically uffd-wp was somehow trying to replace it with a better interface, hopefully.

I have heard about some who does (not me). So I do not make it up, unfortunately.

Quoted 9 lines

If to talk about it, IMHO the most important thing is really not the dirty bit but the write bit: even if you leave both the dirty bits cleared (so the user specified !WRITE_HINT then we grant write bit only), it means when the write happens for sure dirty bit will be set on demand but not soft-dirty since we won't generate a page fault at all. AFAICT it'll be a false negative.

Quoted 3 lines

The old code is safe in that respect on that we always set dirty so we can only have false positive (which is acceptable in this case) but not false negative (which is not).

I agree that the PTE should be left RO in this case. I was just pointing that the user might not be aware of soft-dirty being used, and then it makes sense to treat the (lack of) write-hint appropriately.

Quoted 2 lines

Another option is we call can_change_pte_writable() somehow in the unprotect branch.

I don’t think that I got this one.

tl;dr: I’ll run some numbers (access when PTE-dirty/clear), address the aforementioned issues and send v2. Based on the results we can decide whether it makes sense.

Quoted 24 lines · Nadav Amit

Quoted 11 lines · Peter Xu

⚠ External Email

Quoted 5 lines · Nadav Amit

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.

Quoted 3 lines

Could I ask what's the not supported private mapping you're talking about (and I think you meant "file-backed")? I thought all kinds of private mappings are supported on all modes already?

Yes, I meant file-backed. See vma_can_userfault() for the supported types of memory:

static inline bool vma_can_userfault(struct vm_area_struct *vma, unsigned long vm_flags)
{
if (vm_flags & VM_UFFD_MINOR)
return is_vm_hugetlb_page(vma) || vma_is_shmem(vma);

Quoted 12 lines

#ifndef CONFIG_PTE_MARKER_UFFD_WP
/*
* If user requested uffd-wp but not enabled pte markers for
* uffd-wp, then shmem & hugetlbfs are not supported but only
* anonymous.
*/
if ((vm_flags & VM_UFFD_WP) && !vma_is_anonymous(vma)) return false;
#endif
return vma_is_anonymous(vma) || is_vm_hugetlb_page(vma) || vma_is_shmem(vma);
}

I'm confused. Let me ask in another way: do you mean !VM_SHARED when you say "private"?

Both shmem & hugetlb support private mappings for all three modes, afaict.

Quoted 7 lines

Quoted 4 lines

Well I never thought anyone would be using soft-dirty with uffd because logically uffd-wp was somehow trying to replace it with a better interface, hopefully.

I have heard about some who does (not me). So I do not make it up, unfortunately.

If in any form you can get the reason of using soft-dirty in that use case please kindly share more. I think it could be that uffd-wp is not working as good as soft-dirty in some way but we can think about it when there's a valid use case, so ultimately it's more possible for a full replacement.

Sync messages can be a problem and they're indeed slow, soft-dirty is by nature async. But still I'd like to check in case you know.

Quoted 1 line

Both shmem & hugetlb support private mappings for all three modes, afaict.

Sorry, let me more clear. I meant private (!VM_SHARED) file-backed memory.

⚠ External Email

Quoted 3 lines · Nadav Amit

Quoted 1 line

Both shmem & hugetlb support private mappings for all three modes, afaict.

Sorry, let me more clear. I meant private (!VM_SHARED) file-backed memory.

I normally use "file-backed" to stand for "shmem + hugetlb" in any uffd context.

If you meant anything outside shmem/hugetlb for uffd (e.g. on xfs), then we never support them anyway, do we?

Hmm.. What am I ultimately missing?

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

Quoted 15 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Quoted 2 lines

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

I reply to my own email, but this mostly addresses the concerns that Peter has raised.

So I ran the test below on my Haswell (x86), which showed two things:

1. Accessing an address using a clean PTE or old PTE takes ~500 cycles more than with dirty+young (depending on the access, of course: dirty does not matter for read, dirty+young both matter for write).

2. I made a mistake in my implementation. PTEs are - at least on x86 - created as young with mk_pte(). So the logic should be similar to do_set_pte():

        if (prefault && arch_wants_old_prefaulted_pte())
                entry = pte_mkold(entry);
        else
                entry = pte_sw_mkyoung(entry);

Based on these results, I will send another version for both young and dirty. Let me know if these results are not convincing.

I will add, as we discussed (well, I think I raised these things, so hopefully you agree):

1. On x86, avoid flush if changing WP->RO and PTE is clean.

2. When write-unprotecting entry, if PTE is exclusive, set it as writable. [ I considered not setting it as writable if write-hint is not provided, but with the change in (1), it does not provide any real value. ]

---

#define _GNU_SOURCE
#include <sys/types.h>
#include <stdio.h>
#include <linux/userfaultfd.h>
#include <pthread.h>
#include <errno.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <signal.h>
#include <poll.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/ioctl.h>
#include <stdint.h>
#include <stdbool.h>

#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
		       } while (0)

static inline uint64_t rdtscp(void)

{
	uint64_t rax, rdx;
	uint32_t aux;
	asm volatile ("rdtscp" : "=a" (rax), "=d" (rdx), "=c" (aux):: "memory");
}

int main(int argc, char *argv[])

{
	long uffd;          /* userfaultfd file descriptor */
	char *addr;         /* Start of region handled by userfaultfd */
	unsigned long len;  /* Length of region handled by userfaultfd */
	pthread_t thr;      /* ID of thread that handles page faults */
	bool young, dirty, write;
	struct uffdio_api uffdio_api;
	struct uffdio_register uffdio_register;
	int l;
	static char *page = NULL;
	struct uffdio_copy uffdio_copy;
	ssize_t nread;
	int page_size;

	if (argc != 5) {
		fprintf(stderr, "Usage: %s [num-pages] [write] [young] [dirty]\n", argv[0]);
		exit(EXIT_FAILURE);
	}

	page_size = sysconf(_SC_PAGE_SIZE);
	len = strtoul(argv[1], NULL, 0) * page_size;
	write = !!strtoul(argv[2], NULL, 0);
	young = !!strtoul(argv[3], NULL, 0);
	dirty = !!strtoul(argv[4], NULL, 0);

	page = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
		   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

	if (page == MAP_FAILED)
		errExit("mmap");

	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
	if (uffd == -1)
		errExit("userfaultfd");

	uffdio_api.api = UFFD_API;
	uffdio_api.features = (1<<11); //UFFD_FEATURE_EXACT_ADDRESS;
	if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1)
		errExit("ioctl-UFFDIO_API");

	addr = mmap(NULL, len, PROT_READ | PROT_WRITE,
		MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	if (addr == MAP_FAILED)
		errExit("mmap");

	uffdio_register.range.start = (unsigned long) addr;
	uffdio_register.range.len = len;
	uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
		errExit("ioctl-UFFDIO_REGISTER");

	uffdio_copy.src = (unsigned long) page;
	uffdio_copy.mode = 0;
	if (young)
		uffdio_copy.mode |= (1ul << 2);
	if (dirty)
		uffdio_copy.mode |= (1ul << 3);

	uffdio_copy.len = page_size;
	uffdio_copy.copy = 0;

	for (l = 0; l < len; l += page_size) {
		uffdio_copy.dst = (unsigned long)(&addr[l]);
		if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == -1)
			errExit("ioctl-UFFDIO_COPY");
	}

	for (l = 0; l < len; l += page_size) {
		char c;
		uint64_t start;

		start = rdtscp();
		if (write)
			addr[l] = 5;
		else
			c = *(volatile char *)(&addr[l]);
		printf("%ld\n", rdtscp() - start);
	}

	exit(EXIT_SUCCESS);
}

Hi, Nadav,

Quoted 33 lines · Nadav Amit

Quoted 15 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Quoted 2 lines

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

I reply to my own email, but this mostly addresses the concerns that Peter has raised.

So I ran the test below on my Haswell (x86), which showed two things:

1. Accessing an address using a clean PTE or old PTE takes ~500 cycles more than with dirty+young (depending on the access, of course: dirty does not matter for read, dirty+young both matter for write).

2. I made a mistake in my implementation. PTEs are - at least on x86 - created as young with mk_pte(). So the logic should be similar to do_set_pte():

Quoted 7 lines

if (prefault && arch_wants_old_prefaulted_pte())
entry = pte_mkold(entry);
else
entry = pte_sw_mkyoung(entry);

Based on these results, I will send another version for both young and dirty. Let me know if these results are not convincing.

Thanks for trying to verify this idea, but I'm not fully sure this is what my concern was on WRITE_LIKELY.

AFAICT the test below was trying to measure the overhead of hardware setting either access or dirty or both bits when they're not set for read/write.

What I wanted as a justification is whether WRITE_LIKELY would be helpful in any real world scenario at all. AFAIK the only way to prove it so far is to measure any tlb flush difference (probably only on x86, since that tlb code is only compiled on x86) that may trigger with W=0,D=1 but may not trigger with W=0,D=0 (where W stands for "write bit", and D stands for "dirty bit").

It's not about the slowness when D is cleared.

The core thing is (sorry to rephrase, but just hope we're on the same page) we'll set D bit always for all uffd pages so far. Even if we want to change that behavior so we skip setting D bit for RO pages (we'll need to convert the dirty bit into PageDirty though), we'll still always set D bit for writable pages. So we always set D bit as long as possible and we'll never suffer from hardware overhead on setting D bit for uffd pages.

The other worry of having WRITE_HINT is, after we have it we probably need to _not_ apply dirty bit when WRITE_HINT is not set (which is actually a very light ABI change since we used to always set it), then I'll start to worry the hardware setting D bit overhead you just measured because we'll have that overhead when user didn't specify WRITE_HINT with the old code.

So again, I'm totally fine if you want to start with ACCESS_HINT only, but I still don't see why we should need WRITE_HINT too..

Quoted 44 lines · Peter Xu

Hi, Nadav,

Quoted 33 lines · Nadav Amit

Quoted 15 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Using a PTE on x86 with cleared access-bit (aka young-bit) takes ~600 cycles more than when the access bit is set. At the same time, setting the access-bit for memory that is not used (e.g., prefetched) can introduce greater overheads, as the prefetched memory is reclaimed later than it should be.

Userfaultfd currently does not set the access-bit (excluding the huge-pages case). Arguably, it is best to let the user control whether the access bit should be set or not. The expected use is to request userfaultfd to set the access-bit when the copy/wp operation is done to resolve a page-fault, and not to set the access-bit when the memory is prefetched.

Quoted 2 lines

Introduce UFFDIO_[op]_ACCESS_LIKELY to enable userspace to request the young bit to be set.

I reply to my own email, but this mostly addresses the concerns that Peter has raised.

So I ran the test below on my Haswell (x86), which showed two things:

1. Accessing an address using a clean PTE or old PTE takes ~500 cycles more than with dirty+young (depending on the access, of course: dirty does not matter for read, dirty+young both matter for write).

2. I made a mistake in my implementation. PTEs are - at least on x86 - created as young with mk_pte(). So the logic should be similar to do_set_pte():

Quoted 7 lines

if (prefault && arch_wants_old_prefaulted_pte())
entry = pte_mkold(entry);
else
entry = pte_sw_mkyoung(entry);

Based on these results, I will send another version for both young and dirty. Let me know if these results are not convincing.

Quoted 6 lines

Thanks for trying to verify this idea, but I'm not fully sure this is what my concern was on WRITE_LIKELY.

AFAICT the test below was trying to measure the overhead of hardware setting either access or dirty or both bits when they're not set for read/write.

Indeed.

Quoted 8 lines

What I wanted as a justification is whether WRITE_LIKELY would be helpful in any real world scenario at all. AFAIK the only way to prove it so far is to measure any tlb flush difference (probably only on x86, since that tlb code is only compiled on x86) that may trigger with W=0,D=1 but may not trigger with W=0,D=0 (where W stands for "write bit", and D stands for "dirty bit").

Quoted 8 lines

It's not about the slowness when D is cleared.

The core thing is (sorry to rephrase, but just hope we're on the same page) we'll set D bit always for all uffd pages so far. Even if we want to change that behavior so we skip setting D bit for RO pages (we'll need to convert the dirty bit into PageDirty though), we'll still always set D bit for writable pages. So we always set D bit as long as possible and we'll never suffer from hardware overhead on setting D bit for uffd pages.

Thanks as usual for your clarifications. As you see, I also do my best to be on the same page with, even if from time to time I fail. I had some recent communication challenges on lkml, so I hope that you understand that everything I say is said with full respect, and if I use double-quotes while arguing with you, it is in good spirit, and I really appreciate your feedback.

Ok. So there is a lot to digest in what you just said, and I politely disagree with some of the assertions that you made. You focus on discussing the issue of whether we set the dirty bit for RO pages, which in my opinion is so intuitively wrong. But, I think that discussing this issue really digress us from the benefits of not setting the D-bit when it is unnecessary for RW pages, which is the main question.

But before we get to it, I want to argue with some of the “facts” that you present:

1. "D bit always for all uffd pages” - This is true for almost all UFFD operations, but not true to write-unprotect. The moment we use David’s MM_CP_TRY_CHANGE_WRITABLE in UFFD, the PTE would be writable but not dirty. [Yes, the patches that I sent do not deal with that: as I noted before, I want to send it as part of v2.] Arguably, one of the places that setting the D-bit matters the most if when you change PTE from RO->RW. And anyhow, as you can see the API is inconsistent.

2. "we'll still always set D bit for writable pages” - To continue my previous bullet - why? Why would we? Besides uffd-wrunprotect path, why would we always set it for MCOPY_CONTINUE? (more to follow on this one)

3. "measure any tlb flush difference … that may trigger with W=0,D=1 but may not trigger with W=0,D=0” - This is really a boring case, which even if was underoptimized could have been resolved by its own. This is certainly not the motivation for the write hints.

So please allow me to go back to the reasons for why you want write-hints, and RO entries would be mostly left out and implicitly included in the other arguments - which are mainly about RW entries:

1. You can avoid TLB flushes when write-protecting clean writable PTEs (on x86). Such PTEs can be created when userspace monitor prefaults or speculatively write-unprotects memory that might be needed. When a monitor removes the mapping, using MADV_DONTNEED, it would not need to flush anything.

2. Hopefully you agree that write-hints are needed if during uffd-write-unprotect

we actually write-unprotect the PTE (not just clearing
the logical flag). So you do need write-hint for zero-page (to get a clear-page) and for write-unprotect, so why not to be consistent and provide it for all operations?

3. For UFFDIO_CONTINUE. Admittedly, I am not very familiar with UFFDIO_CONTINUE, but from the discussion (and skimming the code) I understood that you can be used for prefetching. In such case, why would you assume that the page is dirty?

4. It allows you to treat softdirty properly. If softdirty is on, presumably, if you did not get a WRITE_HINT, you would keep it writeprotected.

5. Consistency with UFFDIO_ZERO that needs a write-hint to clear a page.

Now I will “kill myself” over support of write-hints. Not everything that I mentioned here is in v1 that I sent.

But, if you decide you do not want write-hints, I would still need to leave the UFFDIO_ZERO write-hints (that clear the page) and to find some solution for UFFDIO_WRITEPROTECT (that should set the dirty-bit for better performance of WP page-fault handling).

If you decide you do want it, I would run some tests to check that indeed the access-time of UFFDIO_WRITEPROTECT reflects the fact no TLB flush was needed.

Quoted 5 lines

The other worry of having WRITE_HINT is, after we have it we probably need to _not_ apply dirty bit when WRITE_HINT is not set (which is actually a very light ABI change since we used to always set it), then I'll start to worry the hardware setting D bit overhead you just measured because we'll have that overhead when user didn't specify WRITE_HINT with the old code.

Good point, which I have already got to. So, because I was mistaken on the ACCESS_HINT understanding (young-bit is set by default), this would mean that the hints should only be regarded when the “hints” feature is enabled for backward compatibility. I got this code ready for v2.

Quoted 4 lines

So again, I'm totally fine if you want to start with ACCESS_HINT only, but I still don't see why we should need WRITE_HINT too..

I really hate how the “features” evolve that I think it is better to decide now.

I think that in the lack of agreement, the best thing to do is to put all of the write-hints now in the API, and only regard them for UFFDIO_ZERO (which would clear the page) and UFFDIO_WRITE(un)PROTECT (that would set the old bit).

Again, thanks for the feedback, and hopefully (at least) I understood you this time.

Quoted 1 line · Nadav Amit

On Jul 12, 2022, at 7:56 AM, Peter Xu <[email protected]> wrote:

What you suggested at the end sounds good to me, thanks for being persistent. The only thing uncertain seems to be whether we need WRITE_HINT for UFFDIO_WRITEPROTECT, all the rest sounds a good plan.

From: Nadav Amit <[email protected]>

When userfaultfd provides a zeropage in response to ioctl, it provides a readonly alias to the zero page. If the page is later written (which is the likely scenario), page-fault occurs and the page-fault allocator allocates a page and rewires the page-tables.

This is an expensive flow for cases in which a page is likely be written to. Users can use the copy ioctl to initialize zero page (by copying zeros), but this is also wasteful.

Allow userfaultfd users to efficiently map initialized zero-pages that are writable. IF UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY is provided would map a clear page instead of an alias to the zero page.

For consistency, introduce also UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY.

Suggested-by David Hildenbrand <[email protected]>

Cc Mike Kravetz <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Axel Rasmussen <[email protected]>

Cc Peter Xu <[email protected]>

Cc Mike Rapoport <[email protected]>

Signed-off-by Nadav Amit <[email protected]>