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.kun...@gmail.com>
Tested-by: Brendan Jackman <jackm...@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 <jackm...@google.com>
