Re: [RFC PATCH v2 1/5] userfaultfd: introduce uffd_flags

From: David Hildenbrand <david@redhat.com>
To: Nadav Amit <nadav.amit@gmail.com>, linux-mm@kvack.org
Cc: Nadav Amit <namit@vmware.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Hugh Dickins <hughd@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Peter Xu <peterx@redhat.com>, Mike Rapoport <rppt@linux.ibm.com>
Subject: Re: [RFC PATCH v2 1/5] userfaultfd: introduce uffd_flags
Date: Tue, 21 Jun 2022 10:41:19 +0200	[thread overview]
Message-ID: <f58a7012-3d7a-a1b1-6af0-df971287ec7c@redhat.com> (raw)
In-Reply-To: <20220619233449.181323-2-namit@vmware.com>

On 20.06.22 01:34, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> 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 <mike.kravetz@oracle.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Axel Rasmussen <axelrasmussen@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  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(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index d398f6bf6d74..5daafa54eb3f 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -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;
>  
> @@ -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;

?

[...]

> index eee374c29c85..6331148023c1 100644
> --- a/include/linux/userfaultfd_k.h
> +++ b/include/linux/userfaultfd_k.h
> @@ -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;
>  
> @@ -56,23 +55,29 @@ enum mcopy_atomic_mode {
>  	MCOPY_ATOMIC_CONTINUE,
>  };
>  
> +typedef unsigned int __bitwise uffd_flags_t;
> +

Instead of using 0 when no flags are defined,  add

#define UFFD_FLAGS_NONE		((__force uffd_flags_t)0)


which makes it easier to understand at callsites what's happening


Apart from these two things

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb