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

From: Mike Rapoport <rppt@linux.ibm.com>
To: Nadav Amit <nadav.amit@gmail.com>
Cc: Linux MM <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Hugh Dickins <hughd@google.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Peter Xu <peterx@redhat.com>,
	David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH v2 2/5] userfaultfd: introduce access-likely mode for common operations
Date: Tue, 26 Jul 2022 19:02:54 +0300	[thread overview]
Message-ID: <YuAQLnlBJabK8/CZ@linux.ibm.com> (raw)
In-Reply-To: <DBA8D45D-A336-4217-8B02-7D77F0294BB9@gmail.com>

On Mon, Jul 25, 2022 at 10:18:38AM -0700, Nadav Amit wrote:
> 
> > On Jul 23, 2022, at 2:16 AM, Mike Rapoport <rppt@linux.ibm.com> wrote:
> > 
> > On Mon, Jul 18, 2022 at 04:47:45AM -0700, Nadav Amit wrote:
> >> From: Nadav Amit <namit@vmware.com>
> >> 
> >> Introduce access-hints in userfaultfd. The expectation is that userspace
> >> would set access-hints when a page-fault occurred on a page and would
> >> not provide the access-hint on prefaulted memory. The exact behavior of
> >> the kernel in regard to the hints would not be part of userfaultfd api.
> >> 
> >> At this time the use of the access-hint is only in setting access-bit
> >> similarly to the way it is done in do_set_pte(). In x86, currently PTEs
> >> are always marked as young, including prefetched ones. But on arm64,
> >> PTEs would be marked as old (when access bit is supported).
> >> 
> >> If access hints are not enabled, the kernel would behave as if the
> >> access-hint was provided for backward compatibility.
> >> 
> >> 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                 | 39 ++++++++++++++++++++++++++++----
> >> include/linux/userfaultfd_k.h    |  1 +
> >> include/uapi/linux/userfaultfd.h | 20 +++++++++++++++-
> >> mm/internal.h                    | 13 +++++++++++
> >> mm/memory.c                      | 12 ----------
> >> mm/userfaultfd.c                 | 11 +++++++--
> >> 6 files changed, 77 insertions(+), 19 deletions(-)
> >> 
> >> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> >> index 2ae24327beec..8d8792b27c53 100644
> >> --- a/fs/userfaultfd.c
> >> +++ b/fs/userfaultfd.c
> >> @@ -1708,13 +1708,21 @@ 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;
> >> 
> >> 	uffd_flags = mode_wp ? UFFD_FLAGS_WP : UFFD_FLAGS_NONE;
> >> 
> >> +	if (ctx->features & UFFD_FEATURE_ACCESS_HINTS) {
> >> +		if (uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_LIKELY)
> >> +			uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
> >> +	} else {
> >> +		uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
> >> +	}
> >> +
> > 
> > This is quite a construct and it gets more complex in the following
> > patches. How about making it to a static inline function?
> 
> Possible. There is another option though. I think it would have been
> much cleaner if some flags were in common offsets in the different
> “mode” fields. It might be too late for some fields (WP), but I can
> put these the ACCESS/WRITE fields in the the high bits in fixed
> place for all modes, which would allow to at least reuse the logic.

So unless I'm missing something it'll be

	if (ctx->features & UFFD_FEATURE_ACCESS_HINTS)
		uffd_flags |= (uffdio_copy.mode & UFFDIO_COPY_MODE_ACCESS_MASK);
	else
		uffd_flags |= UFFD_FLAGS_ACCESS_MASK;

I still think it's worth wrapping it in static inline with a comments about
common offsets for 'if' clause and backward compatibility for 'else'
clause.

> Is that ok?
> 

-- 
Sincerely yours,
Mike.