Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Apr 2023 15:41:38 +0300
From:   Mike Rapoport <rppt@linux.ibm.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Leonardo Bras Soares Passos <lsoaresp@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Nadav Amit <nadav.amit@gmail.com>
Subject: Re: [PATCH 22/29] selftests/mm: Move uffd pagemap test to unit test
Message-ID: <ZDVVguEBBMfg97cN@linux.ibm.com>
References: <20230330155707.3106228-1-peterx@redhat.com>
 <20230330160821.3107558-1-peterx@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230330160821.3107558-1-peterx@redhat.com>
Precedence: bulk

On Thu, Mar 30, 2023 at 12:08:21PM -0400, Peter Xu wrote:
> Move it over and make it split into two tests, one for pagemap and one for
> the new WP_UNPOPULATED (to be a separate one).
> 
> The thp pagemap test wasn't really working (with MADV_HUGEPAGE).  Let's
> just drop it (since it never really worked anyway..) and leave that for
> later.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> ---
>  tools/testing/selftests/mm/uffd-stress.c     | 166 -------------------
>  tools/testing/selftests/mm/uffd-unit-tests.c | 145 ++++++++++++++++
>  2 files changed, 145 insertions(+), 166 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index 70cb0619354e..50738a993afc 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -671,157 +671,6 @@ static int userfaultfd_minor_test(void)
>  	return stats.missing_faults != 0 || stats.minor_faults != nr_pages;
>  }
> 
> -static int pagemap_open(void)
> -{
> -	int fd = open("/proc/self/pagemap", O_RDONLY);
> -
> -	if (fd < 0)
> -		err("open pagemap");
> -
> -	return fd;
> -}
> -
> -/* This macro let __LINE__ works in err() */
> -#define  pagemap_check_wp(value, wp) do {				\
> -		if (!!(value & PM_UFFD_WP) != wp)			\
> -			err("pagemap uffd-wp bit error: 0x%"PRIx64, value); \
> -	} while (0)
> -
> -static int pagemap_test_fork(bool present)
> -{
> -	pid_t child = fork();
> -	uint64_t value;
> -	int fd, result;
> -
> -	if (!child) {
> -		/* Open the pagemap fd of the child itself */
> -		fd = pagemap_open();
> -		value = pagemap_get_entry(fd, area_dst);
> -		/*
> -		 * After fork() uffd-wp bit should be gone as long as we're
> -		 * without UFFD_FEATURE_EVENT_FORK
> -		 */
> -		pagemap_check_wp(value, false);
> -		/* Succeed */
> -		exit(0);
> -	}
> -	waitpid(child, &result, 0);
> -	return result;
> -}
> -
> -static void userfaultfd_wp_unpopulated_test(int pagemap_fd)
> -{
> -	uint64_t value;
> -
> -	/* Test applying pte marker to anon unpopulated */
> -	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, true);
> -
> -	/* Test unprotect on anon pte marker */
> -	wp_range(uffd, (uint64_t)area_dst, page_size, false);
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, false);
> -
> -	/* Test zap on anon marker */
> -	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> -	if (madvise(area_dst, page_size, MADV_DONTNEED))
> -		err("madvise(MADV_DONTNEED) failed");
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, false);
> -
> -	/* Test fault in after marker removed */
> -	*area_dst = 1;
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, false);
> -	/* Drop it to make pte none again */
> -	if (madvise(area_dst, page_size, MADV_DONTNEED))
> -		err("madvise(MADV_DONTNEED) failed");
> -
> -	/* Test read-zero-page upon pte marker */
> -	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> -	*(volatile char *)area_dst;
> -	/* Drop it to make pte none again */
> -	if (madvise(area_dst, page_size, MADV_DONTNEED))
> -		err("madvise(MADV_DONTNEED) failed");
> -}
> -
> -static void userfaultfd_pagemap_test(unsigned int test_pgsize)
> -{
> -	int pagemap_fd;
> -	uint64_t value;
> -
> -	/* Pagemap tests uffd-wp only */
> -	if (!test_uffdio_wp)
> -		return;
> -
> -	/* Not enough memory to test this page size */
> -	if (test_pgsize > nr_pages * page_size)
> -		return;
> -
> -	printf("testing uffd-wp with pagemap (pgsize=%u): ", test_pgsize);
> -	/* Flush so it doesn't flush twice in parent/child later */
> -	fflush(stdout);
> -
> -	uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED);
> -
> -	if (test_pgsize > page_size) {
> -		/* This is a thp test */
> -		if (madvise(area_dst, nr_pages * page_size, MADV_HUGEPAGE))
> -			err("madvise(MADV_HUGEPAGE) failed");
> -	} else if (test_pgsize == page_size) {
> -		/* This is normal page test; force no thp */
> -		if (madvise(area_dst, nr_pages * page_size, MADV_NOHUGEPAGE))
> -			err("madvise(MADV_NOHUGEPAGE) failed");
> -	}
> -
> -	if (uffd_register(uffd, area_dst, nr_pages * page_size,
> -			  false, true, false))
> -		err("register failed");
> -
> -	pagemap_fd = pagemap_open();
> -
> -	/* Smoke test WP_UNPOPULATED first when it's still empty */
> -	if (test_pgsize == page_size)
> -		userfaultfd_wp_unpopulated_test(pagemap_fd);
> -
> -	/* Touch the page */
> -	*area_dst = 1;
> -	wp_range(uffd, (uint64_t)area_dst, test_pgsize, true);
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, true);
> -	/* Make sure uffd-wp bit dropped when fork */
> -	if (pagemap_test_fork(true))
> -		err("Detected stall uffd-wp bit in child");
> -
> -	/* Exclusive required or PAGEOUT won't work */
> -	if (!(value & PM_MMAP_EXCLUSIVE))
> -		err("multiple mapping detected: 0x%"PRIx64, value);
> -
> -	if (madvise(area_dst, test_pgsize, MADV_PAGEOUT))
> -		err("madvise(MADV_PAGEOUT) failed");
> -
> -	/* Uffd-wp should persist even swapped out */
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, true);
> -	/* Make sure uffd-wp bit dropped when fork */
> -	if (pagemap_test_fork(false))
> -		err("Detected stall uffd-wp bit in child");
> -
> -	/* Unprotect; this tests swap pte modifications */
> -	wp_range(uffd, (uint64_t)area_dst, page_size, false);
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, false);
> -
> -	/* Fault in the page from disk */
> -	*area_dst = 2;
> -	value = pagemap_get_entry(pagemap_fd, area_dst);
> -	pagemap_check_wp(value, false);
> -
> -	close(pagemap_fd);
> -	printf("done\n");
> -}
> -
>  static int userfaultfd_stress(void)
>  {
>  	void *area;
> @@ -933,21 +782,6 @@ static int userfaultfd_stress(void)
>  		uffd_stats_report(uffd_stats, nr_cpus);
>  	}
> 
> -	if (test_type == TEST_ANON) {
> -		/*
> -		 * shmem/hugetlb won't be able to run since they have different
> -		 * behavior on fork() (file-backed memory normally drops ptes
> -		 * directly when fork), meanwhile the pagemap test will verify
> -		 * pgtable entry of fork()ed child.
> -		 */
> -		userfaultfd_pagemap_test(page_size);
> -		/*
> -		 * Hard-code for x86_64 for now for 2M THP, as x86_64 is
> -		 * currently the only one that supports uffd-wp
> -		 */
> -		userfaultfd_pagemap_test(page_size * 512);
> -	}
> -
>  	return userfaultfd_zeropage_test() || userfaultfd_sig_test()
>  		|| userfaultfd_events_test() || userfaultfd_minor_test();
>  }
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index 007145063363..bcd67bd4ec90 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -200,7 +200,152 @@ static bool uffd_feature_supported(uffd_test_case_t *test)
>  	    test->uffd_feature_required;
>  }
> 
> +static int pagemap_open(void)
> +{
> +	int fd = open("/proc/self/pagemap", O_RDONLY);
> +
> +	if (fd < 0)
> +		err("open pagemap");
> +
> +	return fd;
> +}
> +
> +/* This macro let __LINE__ works in err() */
> +#define  pagemap_check_wp(value, wp) do {				\
> +		if (!!(value & PM_UFFD_WP) != wp)			\
> +			err("pagemap uffd-wp bit error: 0x%"PRIx64, value); \
> +	} while (0)
> +
> +static int pagemap_test_fork(bool present)
> +{
> +	pid_t child = fork();
> +	uint64_t value;
> +	int fd, result;
> +
> +	if (!child) {
> +		/* Open the pagemap fd of the child itself */
> +		fd = pagemap_open();
> +		value = pagemap_get_entry(fd, area_dst);
> +		/*
> +		 * After fork() uffd-wp bit should be gone as long as we're
> +		 * without UFFD_FEATURE_EVENT_FORK
> +		 */
> +		pagemap_check_wp(value, false);
> +		/* Succeed */
> +		exit(0);
> +	}
> +	waitpid(child, &result, 0);
> +	return result;
> +}
> +
> +static void uffd_wp_unpopulated_test(void)
> +{
> +	uint64_t value;
> +	int pagemap_fd;
> +
> +	if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +			  false, true, false))
> +		err("register failed");
> +
> +	pagemap_fd = pagemap_open();
> +
> +	/* Test applying pte marker to anon unpopulated */
> +	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, true);
> +
> +	/* Test unprotect on anon pte marker */
> +	wp_range(uffd, (uint64_t)area_dst, page_size, false);
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, false);
> +
> +	/* Test zap on anon marker */
> +	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> +	if (madvise(area_dst, page_size, MADV_DONTNEED))
> +		err("madvise(MADV_DONTNEED) failed");
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, false);
> +
> +	/* Test fault in after marker removed */
> +	*area_dst = 1;
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, false);
> +	/* Drop it to make pte none again */
> +	if (madvise(area_dst, page_size, MADV_DONTNEED))
> +		err("madvise(MADV_DONTNEED) failed");
> +
> +	/* Test read-zero-page upon pte marker */
> +	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> +	*(volatile char *)area_dst;
> +	/* Drop it to make pte none again */
> +	if (madvise(area_dst, page_size, MADV_DONTNEED))
> +		err("madvise(MADV_DONTNEED) failed");
> +
> +	uffd_test_pass();
> +}
> +
> +static void uffd_pagemap_test(void)
> +{
> +	int pagemap_fd;
> +	uint64_t value;
> +
> +	if (uffd_register(uffd, area_dst, nr_pages * page_size,
> +			  false, true, false))
> +		err("register failed");
> +
> +	pagemap_fd = pagemap_open();
> +
> +	/* Touch the page */
> +	*area_dst = 1;
> +	wp_range(uffd, (uint64_t)area_dst, page_size, true);
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, true);
> +	/* Make sure uffd-wp bit dropped when fork */
> +	if (pagemap_test_fork(true))
> +		err("Detected stall uffd-wp bit in child");
> +
> +	/* Exclusive required or PAGEOUT won't work */
> +	if (!(value & PM_MMAP_EXCLUSIVE))
> +		err("multiple mapping detected: 0x%"PRIx64, value);
> +
> +	if (madvise(area_dst, page_size, MADV_PAGEOUT))
> +		err("madvise(MADV_PAGEOUT) failed");
> +
> +	/* Uffd-wp should persist even swapped out */
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, true);
> +	/* Make sure uffd-wp bit dropped when fork */
> +	if (pagemap_test_fork(false))
> +		err("Detected stall uffd-wp bit in child");
> +
> +	/* Unprotect; this tests swap pte modifications */
> +	wp_range(uffd, (uint64_t)area_dst, page_size, false);
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, false);
> +
> +	/* Fault in the page from disk */
> +	*area_dst = 2;
> +	value = pagemap_get_entry(pagemap_fd, area_dst);
> +	pagemap_check_wp(value, false);
> +
> +	close(pagemap_fd);
> +	uffd_test_pass();
> +}
> +
>  uffd_test_case_t uffd_tests[] = {
> +	{
> +		.name = "pagemap",
> +		.uffd_fn = uffd_pagemap_test,
> +		.mem_targets = MEM_ANON,
> +		.uffd_feature_required = UFFD_FEATURE_PAGEFAULT_FLAG_WP,
> +	},
> +	{
> +		.name = "wp-unpopulated",
> +		.uffd_fn = uffd_wp_unpopulated_test,
> +		.mem_targets = MEM_ANON,
> +		.uffd_feature_required =
> +		UFFD_FEATURE_PAGEFAULT_FLAG_WP | UFFD_FEATURE_WP_UNPOPULATED,
> +	},
>  };
> 
>  int main(int argc, char *argv[])
> -- 
> 2.39.1
> 

-- 
Sincerely yours,
Mike.