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 14:02:30 +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 20/29] selftests/mm: Allow allocate_area() to fail
 properly
Message-ID: <ZDU+Rudfi2BUsMHP@linux.ibm.com>
References: <20230330155707.3106228-1-peterx@redhat.com>
 <20230330160815.3107534-1-peterx@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230330160815.3107534-1-peterx@redhat.com>
Precedence: bulk

On Thu, Mar 30, 2023 at 12:08:15PM -0400, Peter Xu wrote:
> Mostly to detect hugetlb allocation errors and skip hugetlb tests when
> pages are not allocated.

Wouldn't we want to skip anon and shmem tests as well for consistency?
 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/mm/uffd-common.c | 32 +++++++++++++++++-------
>  tools/testing/selftests/mm/uffd-common.h |  4 +--
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index 92b7e00efa8a..ae6b61144b53 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -44,10 +44,13 @@ static void anon_release_pages(char *rel_area)
>  		err("madvise(MADV_DONTNEED) failed");
>  }
> 
> -static void anon_allocate_area(void **alloc_area, bool is_src)
> +static int anon_allocate_area(void **alloc_area, bool is_src)
>  {
>  	*alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
>  			   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +	if (*alloc_area == MAP_FAILED)
> +		err("ENOMEM");
> +	return 0;
>  }
> 
>  static void noop_alias_mapping(__u64 *start, size_t len, unsigned long offset)
> @@ -65,7 +68,7 @@ static void hugetlb_release_pages(char *rel_area)
>  	}
>  }
> 
> -static void hugetlb_allocate_area(void **alloc_area, bool is_src)
> +static int hugetlb_allocate_area(void **alloc_area, bool is_src)
>  {
>  	off_t size = nr_pages * page_size;
>  	off_t offset = is_src ? 0 : size;
> @@ -77,14 +80,16 @@ static void hugetlb_allocate_area(void **alloc_area, bool is_src)
>  			   (map_shared ? MAP_SHARED : MAP_PRIVATE) |
>  			   (is_src ? 0 : MAP_NORESERVE),
>  			   mem_fd, offset);
> -	if (*alloc_area == MAP_FAILED)
> -		err("mmap of hugetlbfs file failed");
> +	if (*alloc_area == MAP_FAILED) {
> +		*alloc_area = NULL;
> +		return -errno;
> +	}
> 
>  	if (map_shared) {
>  		area_alias = mmap(NULL, size, PROT_READ | PROT_WRITE,
>  				  MAP_SHARED, mem_fd, offset);
>  		if (area_alias == MAP_FAILED)
> -			err("mmap of hugetlb file alias failed");
> +			return -errno;
>  	}
> 
>  	if (is_src) {
> @@ -96,6 +101,7 @@ static void hugetlb_allocate_area(void **alloc_area, bool is_src)
>  		*alloc_area_alias = area_alias;
> 
>  	close(mem_fd);
> +	return 0;
>  }
> 
>  static void hugetlb_alias_mapping(__u64 *start, size_t len, unsigned long offset)
> @@ -112,7 +118,7 @@ static void shmem_release_pages(char *rel_area)
>  		err("madvise(MADV_REMOVE) failed");
>  }
> 
> -static void shmem_allocate_area(void **alloc_area, bool is_src)
> +static int shmem_allocate_area(void **alloc_area, bool is_src)
>  {
>  	void *area_alias = NULL;
>  	size_t bytes = nr_pages * page_size, hpage_size = read_pmd_pagesize();
> @@ -150,6 +156,7 @@ static void shmem_allocate_area(void **alloc_area, bool is_src)
>  		area_dst_alias = area_alias;
> 
>  	close(mem_fd);
> +	return 0;
>  }
> 
>  static void shmem_alias_mapping(__u64 *start, size_t len, unsigned long offset)
> @@ -282,14 +289,19 @@ static void uffd_test_ctx_clear(void)
>  	munmap_area((void **)&area_remap);
>  }
> 
> -void uffd_test_ctx_init(uint64_t features)
> +int uffd_test_ctx_init(uint64_t features)
>  {
>  	unsigned long nr, cpu;
> +	int ret;
> 
>  	uffd_test_ctx_clear();
> 
> -	uffd_test_ops->allocate_area((void **)&area_src, true);
> -	uffd_test_ops->allocate_area((void **)&area_dst, false);
> +	ret = uffd_test_ops->allocate_area((void **)&area_src, true);
> +	if (ret)
> +		return ret;
> +	ret = uffd_test_ops->allocate_area((void **)&area_dst, false);
> +	if (ret)
> +		return ret;
> 
>  	userfaultfd_open(&features);
> 
> @@ -337,6 +349,8 @@ void uffd_test_ctx_init(uint64_t features)
>  	for (cpu = 0; cpu < nr_cpus; cpu++)
>  		if (pipe2(&pipefd[cpu * 2], O_CLOEXEC | O_NONBLOCK))
>  			err("pipe");
> +
> +	return 0;
>  }
> 
>  void wp_range(int ufd, __u64 start, __u64 len, bool wp)
> diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
> index f4bc73ce3b48..51ec75f6d0c1 100644
> --- a/tools/testing/selftests/mm/uffd-common.h
> +++ b/tools/testing/selftests/mm/uffd-common.h
> @@ -80,7 +80,7 @@ struct uffd_stats {
>  };
> 
>  struct uffd_test_ops {
> -	void (*allocate_area)(void **alloc_area, bool is_src);
> +	int (*allocate_area)(void **alloc_area, bool is_src);
>  	void (*release_pages)(char *rel_area);
>  	void (*alias_mapping)(__u64 *start, size_t len, unsigned long offset);
>  	void (*check_pmd_mapping)(void *p, int expect_nr_hpages);
> @@ -101,7 +101,7 @@ extern uffd_test_ops_t hugetlb_uffd_test_ops;
>  extern uffd_test_ops_t *uffd_test_ops;
> 
>  void uffd_stats_report(struct uffd_stats *stats, int n_cpus);
> -void uffd_test_ctx_init(uint64_t features);
> +int uffd_test_ctx_init(uint64_t features);
>  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);
> -- 
> 2.39.1
> 

-- 
Sincerely yours,
Mike.