Re: [PATCH 19/29] selftests/mm: Let uffd_handle_page_fault() takes wp parameter

[Mike Rapoport]

On Thu, Mar 30, 2023 at 12:08:12PM -0400, Peter Xu wrote:
> Subject: selftests/mm: Let uffd_handle_page_fault() takes wp parameter

Nit:                                                 ^ take

> Make the handler optionally apply WP bit when resolving page faults for
> either missing or minor page faults.  This move towards removing global

Nit:                                        ^ moves

> test_uffdio_wp outside of the common code.
> 
> For this, slightly abuse uffd_stats to keep one more parameter on whether
> we'd like to resolve page faults with WP bit set.  Note that only the name
> is abused, it'll be better to be called uffd_args or similar but let's not
> bother for now.

Maybe one of the first commits in the series should have been
s/uffd_stats/uffd_args/g, but I realize that it's PITA, so I won't insist.
 
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
>  tools/testing/selftests/mm/uffd-common.c | 17 +++++++++--------
>  tools/testing/selftests/mm/uffd-common.h |  6 ++++--
>  tools/testing/selftests/mm/uffd-stress.c | 16 ++++++++++------
>  3 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index 025e40ffc7bf..92b7e00efa8a 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -353,7 +353,7 @@ void wp_range(int ufd, __u64 start, __u64 len, bool wp)
>  		err("clear WP failed: address=0x%"PRIx64, (uint64_t)start);
>  }
> 
> -static void continue_range(int ufd, __u64 start, __u64 len)
> +static void continue_range(int ufd, __u64 start, __u64 len, bool wp)
>  {
>  	struct uffdio_continue req;
>  	int ret;
> @@ -361,7 +361,7 @@ static void continue_range(int ufd, __u64 start, __u64 len)
>  	req.range.start = start;
>  	req.range.len = len;
>  	req.mode = 0;
> -	if (test_uffdio_wp)
> +	if (wp)
>  		req.mode |= UFFDIO_CONTINUE_MODE_WP;
> 
>  	if (ioctl(ufd, UFFDIO_CONTINUE, &req))
> @@ -429,7 +429,8 @@ void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_stats *stats)
>  				    area_dst_alias));
>  		for (b = 0; b < page_size; ++b)
>  			area[b] = ~area[b];
> -		continue_range(uffd, msg->arg.pagefault.address, page_size);
> +		continue_range(uffd, msg->arg.pagefault.address, page_size,
> +			       stats->apply_wp);
>  		stats->minor_faults++;
>  	} else {
>  		/*
> @@ -459,7 +460,7 @@ void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_stats *stats)
>  		offset = (char *)(unsigned long)msg->arg.pagefault.address - area_dst;
>  		offset &= ~(page_size-1);
> 
> -		if (copy_page(uffd, offset))
> +		if (copy_page(uffd, offset, stats->apply_wp))
>  			stats->missing_faults++;
>  	}
>  }
> @@ -555,7 +556,7 @@ static void wake_range(int ufd, unsigned long addr, unsigned long len)
>  			addr), exit(1);
>  }
> 
> -int __copy_page(int ufd, unsigned long offset, bool retry)
> +int __copy_page(int ufd, unsigned long offset, bool retry, bool wp)
>  {
>  	struct uffdio_copy uffdio_copy;
> 
> @@ -564,7 +565,7 @@ int __copy_page(int ufd, unsigned long offset, bool retry)
>  	uffdio_copy.dst = (unsigned long) area_dst + offset;
>  	uffdio_copy.src = (unsigned long) area_src + offset;
>  	uffdio_copy.len = page_size;
> -	if (test_uffdio_wp)
> +	if (wp)
>  		uffdio_copy.mode = UFFDIO_COPY_MODE_WP;
>  	else
>  		uffdio_copy.mode = 0;
> @@ -587,8 +588,8 @@ int __copy_page(int ufd, unsigned long offset, bool retry)
>  	return 0;
>  }
> 
> -int copy_page(int ufd, unsigned long offset)
> +int copy_page(int ufd, unsigned long offset, bool wp)
>  {
> -	return __copy_page(ufd, offset, false);
> +	return __copy_page(ufd, offset, false, wp);
>  }
> 
> diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
> index 47565b2f2dee..f4bc73ce3b48 100644
> --- a/tools/testing/selftests/mm/uffd-common.h
> +++ b/tools/testing/selftests/mm/uffd-common.h
> @@ -72,6 +72,8 @@
>  /* Userfaultfd test statistics */
>  struct uffd_stats {
>  	int cpu;
> +	/* Whether apply wr-protects when installing pages */
> +	bool apply_wp;
>  	unsigned long missing_faults;
>  	unsigned long wp_faults;
>  	unsigned long minor_faults;
> @@ -104,8 +106,8 @@ void userfaultfd_open(uint64_t *features);
>  int uffd_read_msg(int ufd, struct uffd_msg *msg);
>  void wp_range(int ufd, __u64 start, __u64 len, bool wp);
>  void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_stats *stats);
> -int __copy_page(int ufd, unsigned long offset, bool retry);
> -int copy_page(int ufd, unsigned long offset);
> +int __copy_page(int ufd, unsigned long offset, bool retry, bool wp);
> +int copy_page(int ufd, unsigned long offset, bool wp);
>  void *uffd_poll_thread(void *arg);
> 
>  #define TEST_ANON	1
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index 54fc9b4ffa3c..70cb0619354e 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -97,6 +97,7 @@ static void uffd_stats_reset(struct uffd_stats *uffd_stats,
> 
>  	for (i = 0; i < n_cpus; i++) {
>  		uffd_stats[i].cpu = i;
> +		uffd_stats[i].apply_wp = test_uffdio_wp;
>  		uffd_stats[i].missing_faults = 0;
>  		uffd_stats[i].wp_faults = 0;
>  		uffd_stats[i].minor_faults = 0;
> @@ -156,7 +157,7 @@ static void *locking_thread(void *arg)
> 
>  static int copy_page_retry(int ufd, unsigned long offset)
>  {
> -	return __copy_page(ufd, offset, true);
> +	return __copy_page(ufd, offset, true, test_uffdio_wp);
>  }
> 
>  pthread_mutex_t uffd_read_mutex = PTHREAD_MUTEX_INITIALIZER;
> @@ -309,7 +310,7 @@ static void sighndl(int sig, siginfo_t *siginfo, void *ptr)
>   * This also tests UFFD_FEATURE_EVENT_FORK event along with the signal
>   * feature. Using monitor thread, verify no userfault events are generated.
>   */
> -static int faulting_process(int signal_test)
> +static int faulting_process(int signal_test, bool wp)
>  {
>  	unsigned long nr;
>  	unsigned long long count;
> @@ -344,7 +345,7 @@ static int faulting_process(int signal_test)
>  					if (steps == 1) {
>  						/* This is a MISSING request */
>  						steps++;
> -						if (copy_page(uffd, offset))
> +						if (copy_page(uffd, offset, wp))
>  							signalled++;
>  					} else {
>  						/* This is a WP request */
> @@ -508,6 +509,7 @@ static int userfaultfd_events_test(void)
>  			  true, test_uffdio_wp, false))
>  		err("register failure");
> 
> +	stats.apply_wp = test_uffdio_wp;
>  	if (pthread_create(&uffd_mon, &attr, uffd_poll_thread, &stats))
>  		err("uffd_poll_thread create");
> 
> @@ -516,7 +518,7 @@ static int userfaultfd_events_test(void)
>  		err("fork");
> 
>  	if (!pid)
> -		exit(faulting_process(0));
> +		exit(faulting_process(0, test_uffdio_wp));
> 
>  	waitpid(pid, &err, 0);
>  	if (err)
> @@ -552,11 +554,12 @@ static int userfaultfd_sig_test(void)
>  			  true, test_uffdio_wp, false))
>  		err("register failure");
> 
> -	if (faulting_process(1))
> +	if (faulting_process(1, test_uffdio_wp))
>  		err("faulting process failed");
> 
>  	uffd_test_ops->release_pages(area_dst);
> 
> +	stats.apply_wp = test_uffdio_wp;
>  	if (pthread_create(&uffd_mon, &attr, uffd_poll_thread, &stats))
>  		err("uffd_poll_thread create");
> 
> @@ -565,7 +568,7 @@ static int userfaultfd_sig_test(void)
>  		err("fork");
> 
>  	if (!pid)
> -		exit(faulting_process(2));
> +		exit(faulting_process(2, test_uffdio_wp));
> 
>  	waitpid(pid, &err, 0);
>  	if (err)
> @@ -629,6 +632,7 @@ static int userfaultfd_minor_test(void)
>  		       page_size);
>  	}
> 
> +	stats.apply_wp = test_uffdio_wp;
>  	if (pthread_create(&uffd_mon, &attr, uffd_poll_thread, &stats))
>  		err("uffd_poll_thread create");
> 
> -- 
> 2.39.1
> 

-- 
Sincerely yours,
Mike.