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

[David Hildenbrand <david@redhat.com>]

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@gmail.com>
> Acked-by: Peter Xu <peterx@redhat.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