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@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Nadav Amit <namit@vmware.com>,
	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: Sat, 23 Jul 2022 12:16:59 +0300	[thread overview]
Message-ID: <Ytu8i44Co6amcZ7N@linux.ibm.com> (raw)
In-Reply-To: <20220718114748.2623-3-namit@vmware.com>

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?

>  	if (mmget_not_zero(ctx->mm)) {
>  		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
>  				   uffdio_copy.len, &ctx->mmap_changing,
> @@ -1765,9 +1773,17 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx,
>  	if (ret)
>  		goto out;
>  	ret = -EINVAL;
> -	if (uffdio_zeropage.mode & ~UFFDIO_ZEROPAGE_MODE_DONTWAKE)
> +	if (uffdio_zeropage.mode & ~(UFFDIO_ZEROPAGE_MODE_DONTWAKE|
> +				     UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY))
>  		goto out;
>  
> +	if (ctx->features & UFFD_FEATURE_ACCESS_HINTS) {
> +		if (uffdio_zeropage.mode & UFFDIO_ZEROPAGE_MODE_ACCESS_LIKELY)
> +			uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
> +	} else {
> +		uffd_flags |= UFFD_FLAGS_ACCESS_LIKELY;
> +	}
> +
>  	if (mmget_not_zero(ctx->mm)) {
>  		ret = mfill_zeropage(ctx->mm, uffdio_zeropage.range.start,
>  				     uffdio_zeropage.range.len,

-- 
Sincerely yours,
Mike.