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.
