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

[Brendan Jackman <jackmanb@google.com>]

On Sat May 10, 2025 at 4:03 PM UTC, 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>

Tested-by: Brendan Jackman <jackmanb@google.com

Using this hacked to enable the uffd tests (I disable them normally
because they're flaky):

https://github.com/bjackman/linux/blob/github-base/.github/scripts/run_local.sh

> ---
>  Changes since v2:
>  - redo patch on mm-new branch
>  Changes since v1:
>  - indentation fixes
>  - squash into single patch to assist bisections

Thanks for this.

> -static void retry_copy_page(int ufd, struct uffdio_copy *uffdio_copy,
> -			    unsigned long offset)
> +static void retry_copy_page(uffd_global_test_opts_t *gopts, struct uffdio_copy *uffdio_copy,
> +							unsigned long offset)
>  {
> -	uffd_test_ops->alias_mapping(&uffdio_copy->dst,
> -				     uffdio_copy->len,
> -				     offset);
> -	if (ioctl(ufd, UFFDIO_COPY, uffdio_copy)) {
> +	uffd_test_ops->alias_mapping(gopts,
> +								&uffdio_copy->dst,
> +								uffdio_copy->len,
> +								offset);

Looks like your editor got a bit excited here :D

There are a few other places where this happened too, e.g. the
are_count() declaration and there's a pthread_create_call() that's quite
messed up.

Unfortunately I don't know of any tool that can find/fix these issues
automatically without also messing up the whole file. Could you just
do a visual skim and fix what you can spot?

>  static void sigalrm(int sig)
>  {
>  	if (sig != SIGALRM)
>  		abort();
> -	test_uffdio_copy_eexist = true;
> +	// TODO: Set this without access to global vars
> +	// gopts->test_uffdio_copy_eexist = true;

Did you mean to leave this like that?

> @@ -1734,6 +1737,27 @@ int main(int argc, char *argv[])
>  		}
>  		for (j = 0; j < n_mems; j++) {
>  			mem_type = &mem_types[j];
> +
> +			// Initialize global test options

Wrong comment style here

> +			uffd_global_test_opts_t gopts;
> +
> +			gopts.map_shared = mem_type->shared;
> +			uffd_test_ops = mem_type->mem_ops;
> +			uffd_test_case_ops = test->test_case_ops;
> +
> +			if (mem_type->mem_flag & (MEM_HUGETLB_PRIVATE | MEM_HUGETLB))
> +				gopts.page_size = default_huge_page_size();
> +			else
> +				gopts.page_size = psize();
> +
> +			/* Ensure we have at least 2 pages */
> +			gopts.nr_pages = MAX(UFFD_TEST_MEM_SIZE, gopts.page_size * 2) / gopts.page_size;
> +			/* TODO: remove this global var.. it's so ugly */

That's done :)

> +			gopts.nr_parallel = 1;
> +
> +			/* Initialize test arguments */

(This comment seems like noise? I could be wrong, not a big deal).


Thanks for these improvements. Bit of a hasty review and I'm not really
qualified to comment on the test logic itself, but aside from that and
my nits:

Reviewed-by: Brendan Jackman <jackmanb@google.com>