userfaultfd: support access/write hints

From: Nadav Amit <[email protected]>

As the next patches are going to introduce more information that needs to be propagated regarding handled user requests, introduce uffd_flags that would be used to propagate this information.

Remove the unused UFFD_FLAGS_SET to avoid confusion in the constant names.

Introducing uffd flags also allows to avoid mm/userfaultfd from being using uapi (e.g., UFFDIO_COPY_MODE_WP).

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]>

---
 fs/userfaultfd.c              | 20 +++++++++----
 include/linux/hugetlb.h       |  4 +--
 include/linux/shmem_fs.h      |  8 ++++--
 include/linux/userfaultfd_k.h | 23 +++++++++------
 mm/hugetlb.c                  |  3 +-
 mm/shmem.c                    |  6 ++--
 mm/userfaultfd.c              | 53 ++++++++++++++++++-----------------
 7 files changed, 68 insertions(+), 49 deletions(-)

fs/userfaultfd.c +2 -0

index d398f6bf6d74..5daafa54eb3f 100644
@@ -1700,6 +1700,8 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 	struct uffdio_copy uffdio_copy;
 	struct uffdio_copy __user *user_uffdio_copy;
 	struct userfaultfd_wake_range range;
+	bool mode_wp;
+	uffd_flags_t uffd_flags;
 
 	user_uffdio_copy = (struct uffdio_copy __user *) arg;
 

index eee374c29c85..6331148023c1 100644

include/linux/userfaultfd_k.h +0 -1

@@ -34,7 +34,6 @@
 #define UFFD_NONBLOCK O_NONBLOCK
 
 #define UFFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
-#define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
 
 extern int sysctl_unprivileged_userfaultfd;
 

fs/userfaultfd.c static int userfaultfd_copy(struct userfaultfd_ctx *ctx, · line 1734

Quoted 8 lines

fs/userfaultfd.c +4 -0

@@ -1726,10 +1728,15 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 		goto out;
 	if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP))
 		goto out;
+
+	mode_wp = uffdio_copy.mode & UFFDIO_COPY_MODE_WP;
+
+	uffd_flags = mode_wp ? UFFD_FLAGS_WP : 0;

why not simply

uffd_flags = 0;
if (uffdio_copy.mode & UFFDIO_COPY_MODE_WP)

	uffd_flags |= UFFD_FLAGS_WP;

?

mm/userfaultfd.c +1 -1

@@ -683,30 +681,33 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 
 ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start,
 		     unsigned long src_start, unsigned long len,
-		     atomic_t *mmap_changing, __u64 mode)
+		     atomic_t *mmap_changing, uffd_flags_t uffd_flags)

mm/userfaultfd.c +0 -0

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

⚠ External Email

Quoted 6 lines · Nadav Amit

mm/userfaultfd.c +1 -1

@@ -683,30 +681,33 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 
 ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start,
                  unsigned long src_start, unsigned long len,
-                  atomic_t *mmap_changing, __u64 mode)
+                  atomic_t *mmap_changing, uffd_flags_t uffd_flags)

mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, · line 692

Quoted 10 lines

mm/userfaultfd.c +3 -2

 {
     return __mcopy_atomic(dst_mm, dst_start, src_start, len,
-                           MCOPY_ATOMIC_NORMAL, mmap_changing, mode);
+                           MCOPY_ATOMIC_NORMAL, mmap_changing, uffd_flags);
 }
 
 ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,
-                    unsigned long len, atomic_t *mmap_changing)
+                    unsigned long len, atomic_t *mmap_changing,
+                    uffd_flags_t uffd_flags)

Should this be fed into the last parameter of __mcopy_atomic() below?

Quoted 5 lines

mm/userfaultfd.c +0 -0

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

mm/userfaultfd.c static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm, · line 700

Quoted 4 lines

mm/userfaultfd.c +2 -1

 ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,
-                    unsigned long len, atomic_t *mmap_changing)
+                    unsigned long len, atomic_t *mmap_changing,
+                    uffd_flags_t uffd_flags)

Same question here..

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_COPY_MODE_ACCESS_LIKELY and UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY to enable userspace to request the young bit to be set. Set for UFFDIO_CONTINUE and UFFDIO_ZEROPAGE the bit unconditionally since the former is only used to resolve page-faults and the latter would not benefit from not setting the access-bit.

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]>

---
 fs/userfaultfd.c                 | 23 ++++++++++++++++-------
 include/linux/userfaultfd_k.h    |  1 +
 include/uapi/linux/userfaultfd.h | 20 +++++++++++++++++++-
 mm/userfaultfd.c                 | 18 ++++++++++++++++--
 4 files changed, 52 insertions(+), 10 deletions(-)

fs/userfaultfd.c +1 -1

index 5daafa54eb3f..35a8c4347c54 100644
@@ -1700,7 +1700,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 	struct uffdio_copy uffdio_copy;
 	struct uffdio_copy __user *user_uffdio_copy;
 	struct userfaultfd_wake_range range;
-	bool mode_wp;
+	bool mode_wp, mode_access_likely;
 	uffd_flags_t uffd_flags;
 
 	user_uffdio_copy = (struct uffdio_copy __user *) arg;

fs/userfaultfd.c +1 -0

 	if (mmget_not_zero(ctx->mm)) {
 		ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,

@@ -1871,6 +1877,7 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	struct uffdio_continue uffdio_continue;
 	struct uffdio_continue __user *user_uffdio_continue;
 	struct userfaultfd_wake_range range;
+	uffd_flags_t uffd_flags;
 
 	user_uffdio_continue = (struct uffdio_continue __user *)arg;
 

fs/userfaultfd.c +2 -1

+
 	if (mmget_not_zero(ctx->mm)) {
 		ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,
 				     uffdio_continue.range.len,
-				     &ctx->mmap_changing, 0);
+				     &ctx->mmap_changing, uffd_flags);
 		mmput(ctx->mm);
 	} else {
 		return -ESRCH;

include/linux/userfaultfd_k.h +1 -0

index 6331148023c1..e6ac165ec044 100644
@@ -58,6 +58,7 @@ enum mcopy_atomic_mode {
 typedef unsigned int __bitwise uffd_flags_t;
 
 #define UFFD_FLAGS_WP			((__force uffd_flags_t)BIT(0))
+#define UFFD_FLAGS_ACCESS_LIKELY	((__force uffd_flags_t)BIT(1))
 
 extern int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 				    struct vm_area_struct *dst_vma,

include/uapi/linux/userfaultfd.h +2 -1

index 005e5e306266..d9c8ce9ba777 100644
@@ -38,7 +38,8 @@
 			   UFFD_FEATURE_MINOR_HUGETLBFS |	\
 			   UFFD_FEATURE_MINOR_SHMEM |		\
 			   UFFD_FEATURE_EXACT_ADDRESS |		\
-			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
+			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM |	\
+			   UFFD_FEATURE_ACCESS_HINTS)
 #define UFFD_API_IOCTLS				\
 	((__u64)1 << _UFFDIO_REGISTER |		\
 	 (__u64)1 << _UFFDIO_UNREGISTER |	\

include/uapi/linux/userfaultfd.h +1 -0

 	 */
 #define UFFD_FEATURE_PAGEFAULT_FLAG_WP		(1<<0)
 #define UFFD_FEATURE_EVENT_FORK			(1<<1)

@@ -217,6 +222,7 @@ struct uffdio_api {
 #define UFFD_FEATURE_MINOR_SHMEM		(1<<10)
 #define UFFD_FEATURE_EXACT_ADDRESS		(1<<11)
 #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM		(1<<12)
+#define UFFD_FEATURE_ACCESS_HINTS		(1<<13)
 	__u64 features;
 
 	__u64 ioctls;

include/uapi/linux/userfaultfd.h +5 -0

+	 * This can reduce the time that the first access to the page takes.
+	 * Yet, if set opportunistically to memory that is not used, it might
+	 * extend the time before the unused memory pages are reclaimed.
+	 */
+#define UFFDIO_COPY_MODE_ACCESS_LIKELY		((__u64)1<<3)
 };
 
 struct uffdio_zeropage {

include/uapi/linux/userfaultfd.h +2 -0

+ *
  * NOTE: Write protecting a region (WP=1) is unrelated to page faults,
  * therefore DONTWAKE flag is meaningless with WP=1.  Removing write
  * protection (WP=0) in response to a page fault wakes the faulting

@@ -291,6 +308,7 @@ struct uffdio_writeprotect {
  */
 #define UFFDIO_WRITEPROTECT_MODE_WP		((__u64)1<<0)
 #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE	((__u64)1<<1)
+#define UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY	((__u64)1<<2)
 	__u64 mode;
 };

include/uapi/linux/userfaultfd.h +3 -0

@@ -691,6 +699,9 @@ ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,
 		       unsigned long len, atomic_t *mmap_changing,
 		       uffd_flags_t uffd_flags)
 {
+	/* There is no cost for setting the access bit of a zeropage */
+	uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
+
 	return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_ZEROPAGE,
 			      mmap_changing, 0);
 }

Quoted 8 lines

fs/userfaultfd.c +1 -0

@@ -1871,6 +1877,7 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	struct uffdio_continue uffdio_continue;
 	struct uffdio_continue __user *user_uffdio_continue;
 	struct userfaultfd_wake_range range;
+	uffd_flags_t uffd_flags;
 
 	user_uffdio_continue = (struct uffdio_continue __user *)arg;
 

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

Quoted 5 lines

fs/userfaultfd.c +1 -0

@@ -1898,10 +1905,12 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
 		goto out;
 
+	uffd_flags = UFFD_FLAGS_ACCESS_LIKELY;

Can we add a comment why that makes sense? I think I know why -- someone is stuck waiting for that continue to happen :)

include/uapi/linux/userfaultfd.h line 42

Quoted 11 lines

include/uapi/linux/userfaultfd.h +2 -1

index 005e5e306266..d9c8ce9ba777 100644
@@ -38,7 +38,8 @@
 			   UFFD_FEATURE_MINOR_HUGETLBFS |	\
 			   UFFD_FEATURE_MINOR_SHMEM |		\
 			   UFFD_FEATURE_EXACT_ADDRESS |		\
-			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
+			   UFFD_FEATURE_WP_HUGETLBFS_SHMEM |	\
+			   UFFD_FEATURE_ACCESS_HINTS)

⚠ External Email

Quoted 50 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_COPY_MODE_ACCESS_LIKELY and UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY to enable userspace to request the young bit to be set. Set for UFFDIO_CONTINUE and UFFDIO_ZEROPAGE the bit unconditionally since the former is only used to resolve page-faults and the latter would not benefit from not setting the access-bit.

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]>

---

fs/userfaultfd.c                 | 23 ++++++++++++++++-------
include/linux/userfaultfd_k.h    |  1 +
include/uapi/linux/userfaultfd.h | 20 +++++++++++++++++++-
mm/userfaultfd.c                 | 18 ++++++++++++++++--
4 files changed, 52 insertions(+), 10 deletions(-)

fs/userfaultfd.c +1 -1

index 5daafa54eb3f..35a8c4347c54 100644
@@ -1700,7 +1700,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
     struct uffdio_copy uffdio_copy;
     struct uffdio_copy __user *user_uffdio_copy;
     struct userfaultfd_wake_range range;
-     bool mode_wp;
+     bool mode_wp, mode_access_likely;
     uffd_flags_t uffd_flags;
 
     user_uffdio_copy = (struct uffdio_copy __user *) arg;

fs/userfaultfd.c static int userfaultfd_copy(struct userfaultfd_ctx *ctx, · line 1734

Quoted 10 lines

fs/userfaultfd.c +3 -1

@@ -1726,12 +1726,15 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
     ret = -EINVAL;
     if (uffdio_copy.src + uffdio_copy.len <= uffdio_copy.src)
             goto out;
-     if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP))
+     if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP|
+                              UFFDIO_COPY_MODE_ACCESS_LIKELY))
             goto out;
 
     mode_wp = uffdio_copy.mode & UFFDIO_COPY_MODE_WP;
+     mode_access_likely = uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY;

I *relly* prefer just

if (uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY)

       uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY

[...]

fs/userfaultfd.c static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx, · line 1852

Quoted 3 lines

fs/userfaultfd.c +2 -1

-     uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0);
+     uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0) |
+                  (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0);

Dito.

Quoted 11 lines

     if (mmget_not_zero(ctx->mm)) {
             ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,

fs/userfaultfd.c +1 -0

@@ -1871,6 +1877,7 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
     struct uffdio_continue uffdio_continue;
     struct uffdio_continue __user *user_uffdio_continue;
     struct userfaultfd_wake_range range;
+     uffd_flags_t uffd_flags;
 
     user_uffdio_continue = (struct uffdio_continue __user *)arg;
 

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

Quoted 4 lines

fs/userfaultfd.c +1 -0

@@ -1898,10 +1905,12 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
     if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
             goto out;
 
+     uffd_flags = UFFD_FLAGS_ACCESS_LIKELY;

Can we add a comment why that makes sense? I think I know why -- someone is stuck waiting for that continue to happen :)

Quoted 37 lines

fs/userfaultfd.c +2 -1

+
     if (mmget_not_zero(ctx->mm)) {
             ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,
                                  uffdio_continue.range.len,
-                                  &ctx->mmap_changing, 0);
+                                  &ctx->mmap_changing, uffd_flags);
             mmput(ctx->mm);
     } else {
             return -ESRCH;

include/linux/userfaultfd_k.h +1 -0

index 6331148023c1..e6ac165ec044 100644
@@ -58,6 +58,7 @@ enum mcopy_atomic_mode {
 typedef unsigned int __bitwise uffd_flags_t;
 
 #define UFFD_FLAGS_WP                        ((__force uffd_flags_t)BIT(0))
+#define UFFD_FLAGS_ACCESS_LIKELY     ((__force uffd_flags_t)BIT(1))
 
 extern int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
                                 struct vm_area_struct *dst_vma,

include/uapi/linux/userfaultfd.h +2 -1

index 005e5e306266..d9c8ce9ba777 100644
@@ -38,7 +38,8 @@
                        UFFD_FEATURE_MINOR_HUGETLBFS |       \
                        UFFD_FEATURE_MINOR_SHMEM |           \
                        UFFD_FEATURE_EXACT_ADDRESS |         \
-                        UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
+                        UFFD_FEATURE_WP_HUGETLBFS_SHMEM |    \
+                        UFFD_FEATURE_ACCESS_HINTS)
 #define UFFD_API_IOCTLS                              \
     ((__u64)1 << _UFFDIO_REGISTER |         \
      (__u64)1 << _UFFDIO_UNREGISTER |       \

@@ -203,6 +204,10 @@ struct uffdio_api {
      *

include/uapi/linux/userfaultfd.h struct uffdio_api { · line 210

Quoted 6 lines

include/uapi/linux/userfaultfd.h +4 -0

      * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
      * write-protection mode is supported on both shmem and hugetlbfs.
+      *
+      * UFFD_FEATURE_ACCESS_HINTS indicates that the copy supports
+      * UFFDIO_COPY_MODE_ACCESS_LIKELY supports
+      * UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY.

I think that sentence doesn't make sense.

Quoted 10 lines

include/uapi/linux/userfaultfd.h +1 -0

      */
 #define UFFD_FEATURE_PAGEFAULT_FLAG_WP               (1<<0)
 #define UFFD_FEATURE_EVENT_FORK                      (1<<1)

@@ -217,6 +222,7 @@ struct uffdio_api {
 #define UFFD_FEATURE_MINOR_SHMEM             (1<<10)
 #define UFFD_FEATURE_EXACT_ADDRESS           (1<<11)
 #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM              (1<<12)
+#define UFFD_FEATURE_ACCESS_HINTS            (1<<13)
     __u64 features;
 

include/uapi/linux/userfaultfd.h struct uffdio_copy { · line 270

Quoted 7 lines

include/uapi/linux/userfaultfd.h +2 -0

     __u64 ioctls;

@@ -260,6 +266,13 @@ struct uffdio_copy {
      * copy_from_user will not read the last 8 bytes.
      */
     __s64 copy;
+     /*
+      * UFFDIO_COPY_MODE_ACCESS_LIKELY will set the mapped page as young.

Setting the page young is an implementation detail. Can you phrase it more generically what the effect of that hint might be?

Quoted 11 lines

include/uapi/linux/userfaultfd.h +3 -0

@@ -691,6 +699,9 @@ ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,
                    unsigned long len, atomic_t *mmap_changing,
                    uffd_flags_t uffd_flags)
 {
+     /* There is no cost for setting the access bit of a zeropage */
+     uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
+
     return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_ZEROPAGE,
                           mmap_changing, 0);
 }

include/uapi/linux/userfaultfd.h ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start, · line 714

Quoted 6 lines

include/uapi/linux/userfaultfd.h +2 -0

@@ -699,6 +710,9 @@ ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,
                    unsigned long len, atomic_t *mmap_changing,
                    uffd_flags_t uffd_flags)
 {
+     /* The page is likely to be accessed */
+     uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;

Shoouldn't that be set by the caller already?

From: Nadav Amit <[email protected]>

Commit 9ae0f87d009ca ("mm/shmem: unconditionally set pte dirty in mfill_atomic_install_pte") has set PTEs as dirty as its title indicates. However, setting read-only PTEs as dirty can have several undesired implications.

First, setting read-only PTEs as dirty, can cause these PTEs to become writable during mprotect() syscall. See in change_pte_range():

	/* Avoid taking write faults for known dirty pages */
	if (dirty_accountable && pte_dirty(ptent) &&
			(pte_soft_dirty(ptent) ||
			 !(vma->vm_flags & VM_SOFTDIRTY))) {
		ptent = pte_mkwrite(ptent);
	}

Second, unmapping read-only dirty PTEs often prevents TLB flush batching. See try_to_unmap_one():

	/*
	 * Page is dirty. Flush the TLB if a writable entry
	 * potentially exists to avoid CPU writes after IO
	 * starts and then write it out here.
	 */
	try_to_unmap_flush_dirty();

Similarly batching TLB flushed might be prevented in zap_pte_range():

	if (!PageAnon(page)) {
		if (pte_dirty(ptent)) {
			force_flush = 1;
			set_page_dirty(page);
		}
	...

In general, setting a PTE as dirty seems for read-only entries might be dangerous. It should be reminded the dirty-COW vulnerability mitigation also relies on the dirty bit being set only after COW (although it does not appear to apply to userfaultfd).

To summarize, setting the dirty bit for read-only PTEs is dangerous. But even if we only consider writable pages, always setting the dirty bit or always leaving it clear, does not seem as the best policy. Leaving the bit clear introduces overhead on the first write-access to set the bit. Setting the bit for pages the are eventually not written to can require more TLB flushes.

Let the userfaultfd users control whether PTEs are marked as dirty or clean. Introduce UFFDIO_COPY_MODE_WRITE and UFFDIO_COPY_MODE_WRITE_LIKELY and UFFDIO_WRITEPROTECT_MODE_WRITE_LIKELY to enable userspace to indicate whether pages are likely to be written and set the dirty-bit if they are likely to be written.

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]>

---
 fs/userfaultfd.c                 | 22 ++++++++++++++--------
 include/linux/userfaultfd_k.h    |  1 +
 include/uapi/linux/userfaultfd.h | 27 +++++++++++++++++++--------
 mm/hugetlb.c                     |  3 +++
 mm/shmem.c                       |  3 +++
 mm/userfaultfd.c                 | 11 +++++++++--
 6 files changed, 49 insertions(+), 18 deletions(-)

fs/userfaultfd.c +12 -6

index 35a8c4347c54..a56983b594d5 100644
@@ -1700,7 +1700,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 	struct uffdio_copy uffdio_copy;
 	struct uffdio_copy __user *user_uffdio_copy;
 	struct userfaultfd_wake_range range;
-	bool mode_wp, mode_access_likely;
+	bool mode_wp, mode_access_likely, mode_write_likely;
 	uffd_flags_t uffd_flags;
 
 	user_uffdio_copy = (struct uffdio_copy __user *) arg;

@@ -1727,14 +1727,17 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
 	if (uffdio_copy.src + uffdio_copy.len <= uffdio_copy.src)
 		goto out;
 	if (uffdio_copy.mode & ~(UFFDIO_COPY_MODE_DONTWAKE|UFFDIO_COPY_MODE_WP|
-				 UFFDIO_COPY_MODE_ACCESS_LIKELY))
+				 UFFDIO_COPY_MODE_ACCESS_LIKELY|
+				 UFFDIO_COPY_MODE_WRITE_LIKELY))
 		goto out;
 
 	mode_wp = uffdio_copy.mode & UFFDIO_COPY_MODE_WP;
 	mode_access_likely = uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY;
+	mode_write_likely = uffdio_copy.mode & UFFDIO_COPY_MODE_WRITE_LIKELY;
 
 	uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0) |
-		     (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0);
+		     (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0) |
+		     (mode_write_likely ? UFFD_FLAGS_WRITE_LIKELY : 0);
 
 	if (mmget_not_zero(ctx->mm)) {
 		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,

@@ -1819,7 +1822,7 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
 	struct uffdio_writeprotect uffdio_wp;
 	struct uffdio_writeprotect __user *user_uffdio_wp;
 	struct userfaultfd_wake_range range;
-	bool mode_wp, mode_dontwake, mode_access_likely;
+	bool mode_wp, mode_dontwake, mode_access_likely, mode_write_likely;
 	uffd_flags_t uffd_flags;
 
 	if (atomic_read(&ctx->mmap_changing))

@@ -1838,18 +1841,21 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
 
 	if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
 			       UFFDIO_WRITEPROTECT_MODE_WP |
-			       UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY))
+			       UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY |
+			       UFFDIO_WRITEPROTECT_MODE_WRITE_LIKELY))
 		return -EINVAL;
 
 	mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
 	mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
 	mode_access_likely = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY;
+	mode_write_likely = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WRITE_LIKELY;
 
 	if (mode_wp && mode_dontwake)
 		return -EINVAL;
 
 	uffd_flags = (mode_wp ? UFFD_FLAGS_WP : 0) |
-		     (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0);
+		     (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0) |
+		     (mode_write_likely ? UFFD_FLAGS_WRITE_LIKELY : 0);
 
 	if (mmget_not_zero(ctx->mm)) {
 		ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,

fs/userfaultfd.c +0 -0

 	if (mmget_not_zero(ctx->mm)) {
 		ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,

include/linux/userfaultfd_k.h +1 -0

index e6ac165ec044..261a3fa750d0 100644
@@ -59,6 +59,7 @@ typedef unsigned int __bitwise uffd_flags_t;
 
 #define UFFD_FLAGS_WP			((__force uffd_flags_t)BIT(0))
 #define UFFD_FLAGS_ACCESS_LIKELY	((__force uffd_flags_t)BIT(1))
+#define UFFD_FLAGS_WRITE_LIKELY		((__force uffd_flags_t)BIT(2))
 
 extern int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 				    struct vm_area_struct *dst_vma,

include/uapi/linux/userfaultfd.h +19 -8

index d9c8ce9ba777..6ad93a13282e 100644
@@ -267,12 +267,20 @@ struct uffdio_copy {
 	 */
 	__s64 copy;
 	/*
-	 * UFFDIO_COPY_MODE_ACCESS_LIKELY will set the mapped page as young.
-	 * This can reduce the time that the first access to the page takes.
-	 * Yet, if set opportunistically to memory that is not used, it might
-	 * extend the time before the unused memory pages are reclaimed.
+	 * UFFDIO_COPY_MODE_ACCESS_LIKELY indicates that the memory is likely to
+	 * be accessed in the near future, in contrast to memory that is
+	 * opportunistically copied and might not be accessed. The kernel will
+	 * act accordingly, for instance by setting the access-bit in the PTE to
+	 * reduce the access time to the page.
+	 *
+	 * UFFDIO_COPY_MODE_WRITE_LIKELY indicates that the memory is likely to
+	 * be written to. The kernel will act accordingly, for instance by
+	 * setting the dirty-bit in the PTE to reduce the write time to the
+	 * page. This flag will be silently ignored if UFFDIO_COPY_MODE_WP is
+	 * set.
 	 */
-#define UFFDIO_COPY_MODE_ACCESS_LIKELY		((__u64)1<<3)
+#define UFFDIO_COPY_MODE_ACCESS_LIKELY		((__u64)1<<2)
+#define UFFDIO_COPY_MODE_WRITE_LIKELY		((__u64)1<<3)
 };
 
 struct uffdio_zeropage {

@@ -297,9 +305,11 @@ struct uffdio_writeprotect {
  * UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
  * any wait thread after the operation succeeds.
  *
- * UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY: set the flag to mark the modified
- * memory as young, which can reduce the time that the first access
- * to the page takes.
+ * UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY: set the flag to indicate the memory
+ * is likely to be accessed in the near future.
+ *
+ * UFFDIO_WRITEPROTECT_MODE_WRITE_LIKELY: set the flag to indicate that the
+ * memory is likely to be written to in the near future.
  *
  * NOTE: Write protecting a region (WP=1) is unrelated to page faults,
  * therefore DONTWAKE flag is meaningless with WP=1.  Removing write

@@ -309,6 +319,7 @@ struct uffdio_writeprotect {
 #define UFFDIO_WRITEPROTECT_MODE_WP		((__u64)1<<0)
 #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE	((__u64)1<<1)
 #define UFFDIO_WRITEPROTECT_MODE_ACCESS_LIKELY	((__u64)1<<2)
+#define UFFDIO_WRITEPROTECT_MODE_WRITE_LIKELY	((__u64)1<<3)
 	__u64 mode;
 };
 

mm/hugetlb.c +3 -0

index 2beff8a4bf7c..46814fc7762f 100644
@@ -5962,6 +5962,9 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 		*pagep = NULL;
 	}
 
+	/* The PTE is not marked as dirty unconditionally */
+	SetPageDirty(page);
+
 	/*
 	 * The memory barrier inside __SetPageUptodate makes sure that
 	 * preceding stores to the page contents become visible before

mm/shmem.c +3 -0

index 89c775275bae..7488cd186c32 100644
@@ -2404,6 +2404,9 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,
 	VM_BUG_ON(PageSwapBacked(page));
 	__SetPageLocked(page);
 	__SetPageSwapBacked(page);
+
+	/* The PTE is not marked as dirty unconditionally */
+	SetPageDirty(page);
 	__SetPageUptodate(page);
 
 	ret = -EFAULT;

mm/userfaultfd.c +0 -2

index 140c8d3e946e..3172158d8faa 100644
@@ -70,7 +70,6 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 	pgoff_t offset, max_off;
 
 	_dst_pte = mk_pte(page, dst_vma->vm_page_prot);
-	_dst_pte = pte_mkdirty(_dst_pte);
 	if (page_in_cache && !vm_shared)
 		writable = false;
 

@@ -85,13 +84,18 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 
 	if (writable)
 		_dst_pte = pte_mkwrite(_dst_pte);
-	else

mm/userfaultfd.c +5 -0

+	}
+
 	if (uffd_flags & UFFD_FLAGS_ACCESS_LIKELY)
 		_dst_pte = pte_mkyoung(_dst_pte);
 

@@ -180,6 +184,9 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
 		*pagep = NULL;
 	}
 
+	/* The PTE is not marked as dirty unconditionally */
+	SetPageDirty(page);
+
 	/*
 	 * The memory barrier inside __SetPageUptodate makes sure that
 	 * preceding stores to the page contents become visible before

Signature · 1 line

2.25.1

Hi, Nadav,

Quoted 10 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Commit 9ae0f87d009ca ("mm/shmem: unconditionally set pte dirty in mfill_atomic_install_pte") has set PTEs as dirty as its title indicates. However, setting read-only PTEs as dirty can have several undesired implications.

First, setting read-only PTEs as dirty, can cause these PTEs to become writable during mprotect() syscall. See in change_pte_range():

Quoted 6 lines

	/* Avoid taking write faults for known dirty pages */
	if (dirty_accountable && pte_dirty(ptent) &&
			(pte_soft_dirty(ptent) ||
			 !(vma->vm_flags & VM_SOFTDIRTY))) {
		ptent = pte_mkwrite(ptent);
	}

IMHO this is not really the direct reason to add the feature to "allow user to specify whether dirty bit be set for UFFDIO_COPY/... ioctl", because IIUC what's missing is the pte_uffd_wp() check that I should have added in the shmem+hugetlb uffd-wp series in change_pte_range() but I missed..

But since this is fixed by David's patch to optimize mprotect() altogether which checks pte_uffd_wp() (and afaict that's only needed after the shmem+hugetlb patchset, not before), so I think we're safe now with all the branches.

So IMHO we don't need to mention this as it's kind of misleading. It'll be welcomed if you want to recover the pte_dirty behavior in mfill_atomic_install_pte() but probably this is not the right patch for it?

Quoted 10 lines

Second, unmapping read-only dirty PTEs often prevents TLB flush batching. See try_to_unmap_one():

	/*
	 * Page is dirty. Flush the TLB if a writable entry
	 * potentially exists to avoid CPU writes after IO
	 * starts and then write it out here.
	 */
	try_to_unmap_flush_dirty();

Quoted 8 lines

Similarly batching TLB flushed might be prevented in zap_pte_range():

	if (!PageAnon(page)) {
		if (pte_dirty(ptent)) {
			force_flush = 1;
			set_page_dirty(page);
		}
	...

I still keep the pure question here (which I asked in the private reply to you) on why we'd like only pte_dirty() but not pte_write() && pte_dirty() here. I'll rewrite what I have in the private email to you here again that I think should be the write thing to do here..

if (!PageAnon(page)) {
if (pte_dirty(ptent)) {
set_page_dirty(page);
if (pte_write(ptent))
force_flush = 1;
}
}

I also mentioned the other example of doing mprotect(PROT_READ) upon a dirty pte can also create a pte with dirty=1 and write=0 which should be the same condition we have here with uffd, afaict. So it's at least not a solo problem for uffd, and I still think if we treat this tlb flush issue as a perf bug we should consider fixing up the tlb flush code instead because otherwise the "mprotect(PROT_READ) upon dirty pte" will also be able to hit it.

Meanwhile, I'll have the same comment that this is not helping any reviewer to understand why we need to grant user ability to conditionally set dirty bit from uABI, so I think it's better to drop this paragraph too for this patch alone.

Quoted 6 lines

In general, setting a PTE as dirty seems for read-only entries might be dangerous. It should be reminded the dirty-COW vulnerability mitigation also relies on the dirty bit being set only after COW (although it does not appear to apply to userfaultfd).

Quoted 6 lines

To summarize, setting the dirty bit for read-only PTEs is dangerous. But even if we only consider writable pages, always setting the dirty bit or always leaving it clear, does not seem as the best policy. Leaving the bit clear introduces overhead on the first write-access to set the bit. Setting the bit for pages the are eventually not written to can require more TLB flushes.

And IMHO only this paragraph is the real and proper reasoning for this patch..

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

Quoted 11 lines

fs/userfaultfd.c +2 -2

@@ -1902,10 +1908,10 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	 uffdio_continue.range.start) {
 		goto out;
 	}
-	if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
+	if (uffdio_continue.mode & ~(UFFDIO_CONTINUE_MODE_DONTWAKE))
 		goto out;
 
-	uffd_flags = UFFD_FLAGS_ACCESS_LIKELY;
+	uffd_flags = UFFD_FLAGS_ACCESS_LIKELY | UFFD_FLAGS_WRITE_LIKELY;

Setting dirty by default for CONTINUE may make some sense (unlike young), at least to keep the old behavior of the code where the pte was unconditionally set.

But there's another thought a real CONTINUE user modifies the page cache elsewhere so logically the PageDirty should be set elsewhere already anyways, in most cases when the userapp is updating the page cache with another mapping before installing pgtables for this mapping. Then this dirty is not required, it seems.

If we're going to export this to uABI, I'm wondering maybe it'll be nicer to not apply either young or dirty bit for CONTINUE, because fundamentally losing dirty bit doesn't sound risky, meanwhile the user app should have the best knowledge of what to do (whether page was requested or it's just during a pre-faulting in the background).

Axel may have some better thoughts.

Quoted 12 lines

mm/userfaultfd.c +1 -1

@@ -85,13 +84,18 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
 
 	if (writable)
 		_dst_pte = pte_mkwrite(_dst_pte);
-	else
+	else {
 		/*
 		 * We need this to make sure write bit removed; as mk_pte()
 		 * could return a pte with write bit set.
 		 */
 		_dst_pte = pte_wrprotect(_dst_pte);
 

mm/userfaultfd.c int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, · line 96

Quoted 3 lines

mm/userfaultfd.c +3 -0

+		/* Marking RO entries as dirty can mess with other code */
+		if (uffd_flags & UFFD_FLAGS_WRITE_LIKELY)
+			_dst_pte = pte_mkdirty(_dst_pte);

Hmm.. what about "writable=true" ones?

Quoted 30 lines · Peter Xu

Hi, Nadav,

Quoted 10 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Commit 9ae0f87d009ca ("mm/shmem: unconditionally set pte dirty in mfill_atomic_install_pte") has set PTEs as dirty as its title indicates. However, setting read-only PTEs as dirty can have several undesired implications.

First, setting read-only PTEs as dirty, can cause these PTEs to become writable during mprotect() syscall. See in change_pte_range():

Quoted 6 lines

	/* Avoid taking write faults for known dirty pages */
	if (dirty_accountable && pte_dirty(ptent) &&
			(pte_soft_dirty(ptent) ||
			 !(vma->vm_flags & VM_SOFTDIRTY))) {
		ptent = pte_mkwrite(ptent);
	}

IMHO this is not really the direct reason to add the feature to "allow user to specify whether dirty bit be set for UFFDIO_COPY/... ioctl", because IIUC what's missing is the pte_uffd_wp() check that I should have added in the shmem+hugetlb uffd-wp series in change_pte_range() but I missed..

But since this is fixed by David's patch to optimize mprotect() altogether which checks pte_uffd_wp() (and afaict that's only needed after the shmem+hugetlb patchset, not before), so I think we're safe now with all the branches.

Quoted 3 lines

So IMHO we don't need to mention this as it's kind of misleading. It'll be welcomed if you want to recover the pte_dirty behavior in mfill_atomic_install_pte() but probably this is not the right patch for it?

Sorry, I will remove it from the commit log.

I am not sure whether there should be an additional patch to revert (or partially revert) 9ae0f87d009ca and to be backported. What do you say?

Quoted 25 lines

Quoted 11 lines

Second, unmapping read-only dirty PTEs often prevents TLB flush batching. See try_to_unmap_one():

	/*
	 * Page is dirty. Flush the TLB if a writable entry
	 * potentially exists to avoid CPU writes after IO
	 * starts and then write it out here.
	 */
	try_to_unmap_flush_dirty();

Quoted 8 lines

Similarly batching TLB flushed might be prevented in zap_pte_range():

	if (!PageAnon(page)) {
		if (pte_dirty(ptent)) {
			force_flush = 1;
			set_page_dirty(page);
		}
	...

I still keep the pure question here (which I asked in the private reply to you) on why we'd like only pte_dirty() but not pte_write() && pte_dirty() here. I'll rewrite what I have in the private email to you here again that I think should be the write thing to do here..

Quoted 7 lines

if (!PageAnon(page)) {
if (pte_dirty(ptent)) {
set_page_dirty(page);
if (pte_write(ptent))
force_flush = 1;
}
}

The “Second” part regards a perf issue, not a correctness issue. As I told you offline, I think if makes sense to look both on pte_dirty() and pte_write(), but I am afraid of other architectures than x86, which I know.

After our discussion, I looked at other architectures, and it does look safe. So I will add an additional patch for that (with your suggested-by).

Quoted 9 lines

I also mentioned the other example of doing mprotect(PROT_READ) upon a dirty pte can also create a pte with dirty=1 and write=0 which should be the same condition we have here with uffd, afaict. So it's at least not a solo problem for uffd, and I still think if we treat this tlb flush issue as a perf bug we should consider fixing up the tlb flush code instead because otherwise the "mprotect(PROT_READ) upon dirty pte" will also be able to hit it.

Quoted 4 lines

Meanwhile, I'll have the same comment that this is not helping any reviewer to understand why we need to grant user ability to conditionally set dirty bit from uABI, so I think it's better to drop this paragraph too for this patch alone.

Ok. Point taken.

Quoted 13 lines

Quoted 6 lines

In general, setting a PTE as dirty seems for read-only entries might be dangerous. It should be reminded the dirty-COW vulnerability mitigation also relies on the dirty bit being set only after COW (although it does not appear to apply to userfaultfd).

Quoted 6 lines

To summarize, setting the dirty bit for read-only PTEs is dangerous. But even if we only consider writable pages, always setting the dirty bit or always leaving it clear, does not seem as the best policy. Leaving the bit clear introduces overhead on the first write-access to set the bit. Setting the bit for pages the are eventually not written to can require more TLB flushes.

Quoted 2 lines

And IMHO only this paragraph is the real and proper reasoning for this patch..

Will do.

Quoted 22 lines

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

Quoted 11 lines

fs/userfaultfd.c +2 -2

@@ -1902,10 +1908,10 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
 	 uffdio_continue.range.start) {
 		goto out;
 	}
-	if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
+	if (uffdio_continue.mode & ~(UFFDIO_CONTINUE_MODE_DONTWAKE))
 		goto out;
 
-	uffd_flags = UFFD_FLAGS_ACCESS_LIKELY;
+	uffd_flags = UFFD_FLAGS_ACCESS_LIKELY | UFFD_FLAGS_WRITE_LIKELY;

Setting dirty by default for CONTINUE may make some sense (unlike young), at least to keep the old behavior of the code where the pte was unconditionally set.

But there's another thought a real CONTINUE user modifies the page cache elsewhere so logically the PageDirty should be set elsewhere already anyways, in most cases when the userapp is updating the page cache with another mapping before installing pgtables for this mapping. Then this dirty is not required, it seems.

Quoted 7 lines

If we're going to export this to uABI, I'm wondering maybe it'll be nicer to not apply either young or dirty bit for CONTINUE, because fundamentally losing dirty bit doesn't sound risky, meanwhile the user app should have the best knowledge of what to do (whether page was requested or it's just during a pre-faulting in the background).

Axel may have some better thoughts.

I think that perhaps I did not communicate well enough the reason it makes sense to set the dirty-bit.

Setting the access-bit and dirty-bit takes quite some time. So regardless of whether you set the PageDirty, if you didn’t see access-bit and dirty-bit and later you access the page - you are going to pay ~550 cycles for nothing.

For reference: https://marc.info/?l=linux-kernel&m=146582237922378&w=2

If you want me to allow userspace to provide a hint, let me know.

Quoted 32 lines · Nadav Amit

Quoted 30 lines · Peter Xu

Hi, Nadav,

Quoted 10 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Commit 9ae0f87d009ca ("mm/shmem: unconditionally set pte dirty in mfill_atomic_install_pte") has set PTEs as dirty as its title indicates. However, setting read-only PTEs as dirty can have several undesired implications.

First, setting read-only PTEs as dirty, can cause these PTEs to become writable during mprotect() syscall. See in change_pte_range():

Quoted 6 lines

	/* Avoid taking write faults for known dirty pages */
	if (dirty_accountable && pte_dirty(ptent) &&
			(pte_soft_dirty(ptent) ||
			 !(vma->vm_flags & VM_SOFTDIRTY))) {
		ptent = pte_mkwrite(ptent);
	}

IMHO this is not really the direct reason to add the feature to "allow user to specify whether dirty bit be set for UFFDIO_COPY/... ioctl", because IIUC what's missing is the pte_uffd_wp() check that I should have added in the shmem+hugetlb uffd-wp series in change_pte_range() but I missed..

But since this is fixed by David's patch to optimize mprotect() altogether which checks pte_uffd_wp() (and afaict that's only needed after the shmem+hugetlb patchset, not before), so I think we're safe now with all the branches.

Quoted 8 lines

Quoted 3 lines

So IMHO we don't need to mention this as it's kind of misleading. It'll be welcomed if you want to recover the pte_dirty behavior in mfill_atomic_install_pte() but probably this is not the right patch for it?

Sorry, I will remove it from the commit log.

I am not sure whether there should be an additional patch to revert (or partially revert) 9ae0f87d009ca and to be backported. What do you say?

I think we discussed it offlist, I'd not bother but I don't have a strong opinion. Please feel free to go with your preference.

Just to mention that it might not be a clean revert since at that time we don't have continue and uffd-wp on files, so we may need to add the complete check back if we're going to make it. Please also do proper swap tests for both anon and shmem with things like memcg, and when you posted the patches I can do some tests too.

Quoted 32 lines

Quoted 24 lines

Quoted 10 lines

Second, unmapping read-only dirty PTEs often prevents TLB flush batching. See try_to_unmap_one():

	/*
	 * Page is dirty. Flush the TLB if a writable entry
	 * potentially exists to avoid CPU writes after IO
	 * starts and then write it out here.
	 */
	try_to_unmap_flush_dirty();

Quoted 8 lines

Similarly batching TLB flushed might be prevented in zap_pte_range():

	if (!PageAnon(page)) {
		if (pte_dirty(ptent)) {
			force_flush = 1;
			set_page_dirty(page);
		}
	...

I still keep the pure question here (which I asked in the private reply to you) on why we'd like only pte_dirty() but not pte_write() && pte_dirty() here. I'll rewrite what I have in the private email to you here again that I think should be the write thing to do here..

Quoted 7 lines

if (!PageAnon(page)) {
if (pte_dirty(ptent)) {
set_page_dirty(page);
if (pte_write(ptent))
force_flush = 1;
}
}

Quoted 3 lines

The “Second” part regards a perf issue, not a correctness issue. As I told you offline, I think if makes sense to look both on pte_dirty() and pte_write(), but I am afraid of other architectures than x86, which I know.

I don't really see why this logic is arch-specific. I meant, as long as pte_write() returns a value that means "this page is writable", I still think it's making sense.

Quoted 2 lines

After our discussion, I looked at other architectures, and it does look safe. So I will add an additional patch for that (with your suggested-by).

Only if you agree with what I thought, thanks. And that could be a separate patch for sure out of this one even if to come.

Quoted 5 lines

I think that perhaps I did not communicate well enough the reason it makes sense to set the dirty-bit.

Quoted 8 lines

Setting the access-bit and dirty-bit takes quite some time. So regardless of whether you set the PageDirty, if you didn’t see access-bit and dirty-bit and later you access the page - you are going to pay ~550 cycles for nothing.

For reference: https://marc.info/?l=linux-kernel&m=146582237922378&w=2

If you want me to allow userspace to provide a hint, let me know.

You did communicate well, so probably it's me that didn't. :)

Yes I think it'll be nicer to allow userspace provide the same hint for UFFDIO_CONTINUE, the reason as I explained in the other thread on the young bit (or say ACCESS_LIKELY), that UFFDIO_CONTINUE can (at least in my current qemu impl [1]) proactively be used to install pgtables even if they're code pages and they may not be accessed in the near future. So IMHO we should treat UFFDIO_CONTINUE the same as UFFDIO_COPY, IMHO, from this point of view.

There's just still some differences on young/dirty here so I'm not sure whether the idea applies to both, but I think at least it applies to young bit (ACCESS_LIKELY).

[1] https://github.com/xzpeter/qemu/commit/b7758ad55af42b5364796362e96f4a06b51d9582

Quoted 2 lines

Only if you agree with what I thought, thanks. And that could be a separate patch for sure out of this one even if to come.

I do agree. I did not quite understand your second sentence. I guess you meant not to combine it into this patchset (or at least that’s what I thought and I attribute it to you).

Quoted 25 lines

Quoted 6 lines

I think that perhaps I did not communicate well enough the reason it makes sense to set the dirty-bit.

Quoted 8 lines

Setting the access-bit and dirty-bit takes quite some time. So regardless of whether you set the PageDirty, if you didn’t see access-bit and dirty-bit and later you access the page - you are going to pay ~550 cycles for nothing.

For reference: https://marc.info/?l=linux-kernel&m=146582237922378&w=2

If you want me to allow userspace to provide a hint, let me know.

You did communicate well, so probably it's me that didn't. :)

Yes I think it'll be nicer to allow userspace provide the same hint for UFFDIO_CONTINUE, the reason as I explained in the other thread on the young bit (or say ACCESS_LIKELY), that UFFDIO_CONTINUE can (at least in my current qemu impl [1]) proactively be used to install pgtables even if they're code pages and they may not be accessed in the near future. So IMHO we should treat UFFDIO_CONTINUE the same as UFFDIO_COPY, IMHO, from this point of view.

Quoted 5 lines

There's just still some differences on young/dirty here so I'm not sure whether the idea applies to both, but I think at least it applies to young bit (ACCESS_LIKELY).

[1] https://github.com/xzpeter/qemu/commit/b7758ad55af42b5364796362e96f4a06b51d9582

We have a saying in Hebrew: “When you explain slowly, I understand quickly”. Thanks for explaining. I will add hints for UFFDIO_CONTINUE as well.

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. Introduce UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY, which, when 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]>

---
 fs/userfaultfd.c                 | 14 +++++++++++--
 include/uapi/linux/userfaultfd.h |  2 ++
 mm/userfaultfd.c                 | 36 ++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 2 deletions(-)

fs/userfaultfd.c +2 -0

index a56983b594d5..ff073de78ea8 100644
@@ -1770,6 +1770,8 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
 	struct uffdio_zeropage uffdio_zeropage;
 	struct uffdio_zeropage __user *user_uffdio_zeropage;
 	struct userfaultfd_wake_range range;
+	bool mode_dontwake, mode_access_likely, mode_write_likely;
+	uffd_flags_t uffd_flags;
 
 	user_uffdio_zeropage = (struct uffdio_zeropage __user *) arg;
 

@@ -1788,8 +1790,16 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
 	if (ret)
 		goto out;

fs/userfaultfd.c +3 -0

+
+	uffd_flags = (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0) |
+		     (mode_write_likely ? UFFD_FLAGS_WRITE_LIKELY : 0);
 
 	if (mmget_not_zero(ctx->mm)) {
 		ret = mfill_zeropage(ctx->mm, uffdio_zeropage.range.start,

include/uapi/linux/userfaultfd.h +2 -0

index 6ad93a13282e..b586b7c1e265 100644
@@ -286,6 +286,8 @@ struct uffdio_copy {
 struct uffdio_zeropage {
 	struct uffdio_range range;
 #define UFFDIO_ZEROPAGE_MODE_DONTWAKE		((__u64)1<<0)
+#define UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY	((__u64)1<<2)
+#define UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY	((__u64)1<<3)
 	__u64 mode;
 
 	/*

mm/userfaultfd.c +8 -0

index 3172158d8faa..5dfbb1e80369 100644
@@ -249,6 +249,38 @@ static int mfill_zeropage_pte(struct mm_struct *dst_mm,
 	return ret;
 }
 
+static int mfill_clearpage_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
+			       struct vm_area_struct *dst_vma,
+			       unsigned long dst_addr,
+			       uffd_flags_t uffd_flags)
+{
+	struct page *page;
+	int ret;
+

mm/userfaultfd.c +18 -0

+	if (mem_cgroup_charge(page_folio(page), dst_vma->vm_mm, GFP_KERNEL))
+		goto out_release;
+
+	ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
+				       page, true, uffd_flags);
+	if (ret)
+		goto out_release;
+out:
+	return ret;
+out_release:
+	put_page(page);
+	goto out;
+}
+
 /* Handles UFFDIO_CONTINUE for all shmem VMAs (shared or private). */
 static int mcontinue_atomic_pte(struct mm_struct *dst_mm,
 				pmd_t *dst_pmd,

@@ -511,6 +543,10 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
 			err = mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,
 					       dst_addr, src_addr, page,
 					       uffd_flags);
+		else if (!(uffd_flags & UFFD_FLAGS_WP) &&
+			 (uffd_flags & UFFD_FLAGS_WRITE_LIKELY))
+			err = mfill_clearpage_pte(dst_mm, dst_pmd, dst_vma,
+						  dst_addr, uffd_flags);
 		else
 			err = mfill_zeropage_pte(dst_mm, dst_pmd,
 						 dst_vma, dst_addr, uffd_flags);

Signature · 1 line

2.25.1

⚠ External Email

Quoted 47 lines · Nadav Amit

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. Introduce UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY, which, when 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]>

---

fs/userfaultfd.c                 | 14 +++++++++++--
include/uapi/linux/userfaultfd.h |  2 ++
mm/userfaultfd.c                 | 36 ++++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 2 deletions(-)

fs/userfaultfd.c +2 -0

index a56983b594d5..ff073de78ea8 100644
@@ -1770,6 +1770,8 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
     struct uffdio_zeropage uffdio_zeropage;
     struct uffdio_zeropage __user *user_uffdio_zeropage;
     struct userfaultfd_wake_range range;
+     bool mode_dontwake, mode_access_likely, mode_write_likely;
+     uffd_flags_t uffd_flags;
 
     user_uffdio_zeropage = (struct uffdio_zeropage __user *) arg;
 

@@ -1788,8 +1790,16 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
     if (ret)
             goto out;

fs/userfaultfd.c static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx, · line 1799

Quoted 10 lines

fs/userfaultfd.c +7 -2

     ret = -EINVAL;
-     if (uffdio_zeropage.mode & ~UFFDIO_ZEROPAGE_MODE_DONTWAKE)
-             goto out;
+
+     mode_dontwake = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_DONTWAKE;
+     mode_access_likely = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY;
+     mode_write_likely = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY;
+
+     if (mode_dontwake)
+             return -EINVAL;

Hmm.. Why?

Note that the above uffdio_zeropage.mode check was for invalid mode flags only, and I think that should be kept, but still I don't see why we want to fail UFFDIO_ZEROPAGE_MODE_DONTWAKE users.

Quoted 39 lines

fs/userfaultfd.c +3 -0

+
+     uffd_flags = (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0) |
+                  (mode_write_likely ? UFFD_FLAGS_WRITE_LIKELY : 0);
 
     if (mmget_not_zero(ctx->mm)) {
             ret = mfill_zeropage(ctx->mm, uffdio_zeropage.range.start,

include/uapi/linux/userfaultfd.h +2 -0

index 6ad93a13282e..b586b7c1e265 100644
@@ -286,6 +286,8 @@ struct uffdio_copy {
 struct uffdio_zeropage {
     struct uffdio_range range;
 #define UFFDIO_ZEROPAGE_MODE_DONTWAKE                ((__u64)1<<0)
+#define UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY   ((__u64)1<<2)
+#define UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY    ((__u64)1<<3)
     __u64 mode;
 
     /*

mm/userfaultfd.c +12 -0

index 3172158d8faa..5dfbb1e80369 100644
@@ -249,6 +249,38 @@ static int mfill_zeropage_pte(struct mm_struct *dst_mm,
     return ret;
 }
 
+static int mfill_clearpage_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
+                            struct vm_area_struct *dst_vma,
+                            unsigned long dst_addr,
+                            uffd_flags_t uffd_flags)
+{
+     struct page *page;
+     int ret;
+
+     ret = -ENOMEM;
+     page = alloc_zeroed_user_highpage_movable(dst_vma, dst_addr);
+     if (!page)
+             goto out;

mm/userfaultfd.c static int mfill_zeropage_pte(struct mm_struct *dst_mm, · line 269

Quoted 6 lines

mm/userfaultfd.c +6 -0

+
+     /* The PTE is not marked as dirty unconditionally */
+     SetPageDirty(page);
+     __SetPageUptodate(page);
+
+     ret = -ENOMEM;

Nit: can drop this since ret will always be -ENOMEM here..

Quoted 63 lines · Peter Xu

⚠ External Email

Quoted 47 lines · Nadav Amit

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. Introduce UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY, which, when 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]>

---

fs/userfaultfd.c                 | 14 +++++++++++--
include/uapi/linux/userfaultfd.h |  2 ++
mm/userfaultfd.c                 | 36 ++++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 2 deletions(-)

fs/userfaultfd.c +2 -0

index a56983b594d5..ff073de78ea8 100644
@@ -1770,6 +1770,8 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
     struct uffdio_zeropage uffdio_zeropage;
     struct uffdio_zeropage __user *user_uffdio_zeropage;
     struct userfaultfd_wake_range range;
+     bool mode_dontwake, mode_access_likely, mode_write_likely;
+     uffd_flags_t uffd_flags;
 
     user_uffdio_zeropage = (struct uffdio_zeropage __user *) arg;
 

@@ -1788,8 +1790,16 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
     if (ret)
             goto out;

fs/userfaultfd.c static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx, · line 1799

Quoted 10 lines

fs/userfaultfd.c +7 -2

     ret = -EINVAL;
-     if (uffdio_zeropage.mode & ~UFFDIO_ZEROPAGE_MODE_DONTWAKE)
-             goto out;
+
+     mode_dontwake = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_DONTWAKE;
+     mode_access_likely = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY;
+     mode_write_likely = uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY;
+
+     if (mode_dontwake)
+             return -EINVAL;

Hmm.. Why?

Quoted 3 lines

Note that the above uffdio_zeropage.mode check was for invalid mode flags only, and I think that should be kept, but still I don't see why we want to fail UFFDIO_ZEROPAGE_MODE_DONTWAKE users.

Quoted 40 lines

Quoted 40 lines

fs/userfaultfd.c +3 -0

+
+     uffd_flags = (mode_access_likely ? UFFD_FLAGS_ACCESS_LIKELY : 0) |
+                  (mode_write_likely ? UFFD_FLAGS_WRITE_LIKELY : 0);
 
     if (mmget_not_zero(ctx->mm)) {
             ret = mfill_zeropage(ctx->mm, uffdio_zeropage.range.start,

include/uapi/linux/userfaultfd.h +2 -0

index 6ad93a13282e..b586b7c1e265 100644
@@ -286,6 +286,8 @@ struct uffdio_copy {
 struct uffdio_zeropage {
     struct uffdio_range range;
 #define UFFDIO_ZEROPAGE_MODE_DONTWAKE                ((__u64)1<<0)
+#define UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY   ((__u64)1<<2)
+#define UFFDIO_ZEROPAGE_MODE_WRITE_LIKELY    ((__u64)1<<3)
     __u64 mode;
 
     /*

mm/userfaultfd.c +12 -0

index 3172158d8faa..5dfbb1e80369 100644
@@ -249,6 +249,38 @@ static int mfill_zeropage_pte(struct mm_struct *dst_mm,
     return ret;
 }
 
+static int mfill_clearpage_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
+                            struct vm_area_struct *dst_vma,
+                            unsigned long dst_addr,
+                            uffd_flags_t uffd_flags)
+{
+     struct page *page;
+     int ret;
+
+     ret = -ENOMEM;
+     page = alloc_zeroed_user_highpage_movable(dst_vma, dst_addr);
+     if (!page)
+             goto out;

Quoted 8 lines

mm/userfaultfd.c static int mfill_zeropage_pte(struct mm_struct *dst_mm, · line 269

Quoted 6 lines

mm/userfaultfd.c +6 -0

+
+     /* The PTE is not marked as dirty unconditionally */
+     SetPageDirty(page);
+     __SetPageUptodate(page);
+
+     ret = -ENOMEM;

Nit: can drop this since ret will always be -ENOMEM here..

I noticed. Just thought it is clearer this way, and more robust against future changes.

⚠ External Email

Quoted 4 lines

Quoted 1 line

Nit: can drop this since ret will always be -ENOMEM here..

I noticed. Just thought it is clearer this way, and more robust against future changes.

I'd rather leave that for future, but if you really prefer that no problem on my side too.

Please just still check [1] above and that's the major real comment, just to make sure it's not overlooked..