linux-kernel - Re: [PATCH v7 1/1] selftests/mm/uffd: Refactor non-composite global vars into struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f9eeb98-9881-4c46-93f9-e13419d92b61@redhat.com>
Date: Mon, 18 Aug 2025 09:56:21 +0200
From: David Hildenbrand <david@...hat.com>
To: Ujwal Kundur <ujwal.kundur@...il.com>, akpm@...ux-foundation.org,
 peterx@...hat.com, jackmanb@...gle.com, lorenzo.stoakes@...cle.com,
 Liam.Howlett@...cle.com, vbabka@...e.cz, rppt@...nel.org, surenb@...gle.com,
 mhocko@...e.com, shuah@...nel.org
Cc: linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 1/1] selftests/mm/uffd: Refactor non-composite global
 vars into struct

On 17.08.25 08:52, Ujwal Kundur wrote:
> Refactor macros and non-composite global variable definitions into a
> struct that is defined at the start of a test and is passed around
> instead of relying on global vars.
> 
> Signed-off-by: Ujwal Kundur <ujwal.kundur@...il.com>
> Acked-by: Peter Xu <peterx@...hat.com>
> ---
> Previous versions and discussion at:
> https://lore.kernel.org/all/20250702152057.4067-1-ujwal.kundur@gmail.com/
> 
>   Changes since v6:
>   - rebased on 6.17-rc1 changes (cd79a1d9b08a)
>     - removes unused args and adds the __unused attribute; since change
> 	 is cosmetic-only, carry forward Acked-by tag
>   - verified output remains unchanged using virtme-ng
>   Changes since v5:
>   - ensure uffd_global_test_opts_t instances are initialized
>   - verified output remains unchanged using virtme-ng
>   Changes since v4:
>   - define gopts as global within uffd-stress.c to retain existing
>     sigalrm handler logic
>   Changes since v3:
>   - more formatting fixes
>   Changes since v2:
>   - redo patch on mm-new branch
>   Changes since v1:
>   - indentation fixes
>   - squash into single patch to assist bisections
> 
>   tools/testing/selftests/mm/uffd-common.c     | 275 ++++-----
>   tools/testing/selftests/mm/uffd-common.h     |  78 +--
>   tools/testing/selftests/mm/uffd-stress.c     | 228 ++++----
>   tools/testing/selftests/mm/uffd-unit-tests.c | 561 ++++++++++---------
>   tools/testing/selftests/mm/uffd-wp-mremap.c  |  23 +-
>   5 files changed, 623 insertions(+), 542 deletions(-)

That's a lot of churn, but sounds reasonable. Only skimmed over it and 
found two nits.

> 
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index e309ec886fa7..f4e9a5f43e24 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -7,18 +7,30 @@
>   
>   #include "uffd-common.h"
>   
> -#define BASE_PMD_ADDR ((void *)(1UL << 30))
> -
> -volatile bool test_uffdio_copy_eexist = true;
> -unsigned long nr_parallel, nr_pages, nr_pages_per_cpu, page_size;
> -char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
> -int uffd = -1, uffd_flags, finished, *pipefd, test_type;
> -bool map_shared;
> -bool test_uffdio_wp = true;
> -unsigned long long *count_verify;
>   uffd_test_ops_t *uffd_test_ops;
>   uffd_test_case_ops_t *uffd_test_case_ops;
> -atomic_bool ready_for_fork;
> +
> +#define BASE_PMD_ADDR ((void *)(1UL << 30))
> +
> +/* pthread_mutex_t starts at page offset 0 */
> +pthread_mutex_t *area_mutex(char *area, unsigned long nr, uffd_global_test_opts_t *gopts)
> +{
> +	return (pthread_mutex_t *) (area + nr * gopts->page_size);
> +}
> +
> +/*
> + * count is placed in the page after pthread_mutex_t naturally aligned
> + * to avoid non alignment faults on non-x86 archs.
> + */
> +volatile unsigned long long *area_count(
> +		char *area, unsigned long nr,
> +		uffd_global_test_opts_t *gopts)

You can fit some parameters into the first line to make this look less 
weird.

[...]

>   	}
>   
> diff --git a/tools/testing/selftests/mm/uffd-wp-mremap.c b/tools/testing/selftests/mm/uffd-wp-mremap.c
> index b2b6116e6580..ec860625b25b 100644
> --- a/tools/testing/selftests/mm/uffd-wp-mremap.c
> +++ b/tools/testing/selftests/mm/uffd-wp-mremap.c
> @@ -152,7 +152,11 @@ static bool range_is_swapped(void *addr, size_t size)
>   	return true;
>   }
>   
> -static void test_one_folio(size_t size, bool private, bool swapout, bool hugetlb)
> +static void test_one_folio(uffd_global_test_opts_t *gopts,
> +			   size_t size,
> +			   bool private,
> +			   bool swapout,
> +			   bool hugetlb)

Please avoid that.

static void test_one_folio(uffd_global_test_opts_t *gopts, size_t size,
		bool private, bool swapout, bool hugetlb)

-- 
Cheers

David / dhildenb