| Message ID | 20250228-mm-selftests-v3-4-958e3b6f0203@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: Some cleanups from trying to run them | expand |
On 28/02/25 10:24 pm, Brendan Jackman wrote: > A later commit will bound this variable so it no longer necessarily > matches the number of CPUs. Rename it appropriately. > > Signed-off-by: Brendan Jackman <jackmanb@google.com> > --- > tools/testing/selftests/mm/uffd-common.c | 8 ++++---- > tools/testing/selftests/mm/uffd-common.h | 2 +- > tools/testing/selftests/mm/uffd-stress.c | 28 ++++++++++++++-------------- > tools/testing/selftests/mm/uffd-unit-tests.c | 2 +- > 4 files changed, 20 insertions(+), 20 deletions(-) > > diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c > index 717539eddf98754250e70e564cd9a59f398bd7ea..a72a2ed5e89480ed06c81b034967ed5ae5f8cad5 100644 > --- a/tools/testing/selftests/mm/uffd-common.c > +++ b/tools/testing/selftests/mm/uffd-common.c > @@ -10,7 +10,7 @@ > #define BASE_PMD_ADDR ((void *)(1UL << 30)) > > volatile bool test_uffdio_copy_eexist = true; > -unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; > +unsigned long nr_threads, 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; > @@ -269,7 +269,7 @@ void uffd_test_ctx_clear(void) > size_t i; > > if (pipefd) { > - for (i = 0; i < nr_cpus * 2; ++i) { > + for (i = 0; i < nr_threads * 2; ++i) { > if (close(pipefd[i])) > err("close pipefd"); > } > @@ -365,10 +365,10 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg) > */ > uffd_test_ops->release_pages(area_dst); > > - pipefd = malloc(sizeof(int) * nr_cpus * 2); > + pipefd = malloc(sizeof(int) * nr_threads * 2); > if (!pipefd) > err("pipefd"); > - for (cpu = 0; cpu < nr_cpus; cpu++) > + for (cpu = 0; cpu < nr_threads; cpu++) > if (pipe2(&pipefd[cpu * 2], O_CLOEXEC | O_NONBLOCK)) > err("pipe"); > > diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h > index a70ae10b5f6206daecc8e19ed3e3bbb388e265aa..604e3572fe17280ae346b031e2e867e039578f95 100644 > --- a/tools/testing/selftests/mm/uffd-common.h > +++ b/tools/testing/selftests/mm/uffd-common.h > @@ -98,7 +98,7 @@ struct uffd_test_case_ops { > }; > typedef struct uffd_test_case_ops uffd_test_case_ops_t; > > -extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; > +extern unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size; > extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap; > extern int uffd, uffd_flags, finished, *pipefd, test_type; > extern bool map_shared; > diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c > index ed68436fac62c76e2ca7060c661487f2f8a6ab45..ec842bbb9f18e291fa51de0ed8d1fbf9aaf14372 100644 > --- a/tools/testing/selftests/mm/uffd-stress.c > +++ b/tools/testing/selftests/mm/uffd-stress.c > @@ -179,12 +179,12 @@ static void *background_thread(void *arg) > static int stress(struct uffd_args *args) > { > unsigned long cpu; > - pthread_t locking_threads[nr_cpus]; > - pthread_t uffd_threads[nr_cpus]; > - pthread_t background_threads[nr_cpus]; > + pthread_t locking_threads[nr_threads]; > + pthread_t uffd_threads[nr_threads]; > + pthread_t background_threads[nr_threads]; > > finished = 0; > - for (cpu = 0; cpu < nr_cpus; cpu++) { > + for (cpu = 0; cpu < nr_threads; cpu++) { > if (pthread_create(&locking_threads[cpu], &attr, > locking_thread, (void *)cpu)) > return 1; > @@ -202,7 +202,7 @@ static int stress(struct uffd_args *args) > background_thread, (void *)cpu)) > return 1; > } > - for (cpu = 0; cpu < nr_cpus; cpu++) > + for (cpu = 0; cpu < nr_threads; cpu++) > if (pthread_join(background_threads[cpu], NULL)) > return 1; > > @@ -218,11 +218,11 @@ static int stress(struct uffd_args *args) > uffd_test_ops->release_pages(area_src); > > finished = 1; > - for (cpu = 0; cpu < nr_cpus; cpu++) > + for (cpu = 0; cpu < nr_threads; cpu++) > if (pthread_join(locking_threads[cpu], NULL)) > return 1; > > - for (cpu = 0; cpu < nr_cpus; cpu++) { > + for (cpu = 0; cpu < nr_threads; cpu++) { > char c; > if (bounces & BOUNCE_POLL) { > if (write(pipefd[cpu*2+1], &c, 1) != 1) > @@ -245,11 +245,11 @@ static int userfaultfd_stress(void) > { > void *area; > unsigned long nr; > - struct uffd_args args[nr_cpus]; > + struct uffd_args args[nr_threads]; > uint64_t mem_size = nr_pages * page_size; > int flags = 0; > > - memset(args, 0, sizeof(struct uffd_args) * nr_cpus); > + memset(args, 0, sizeof(struct uffd_args) * nr_threads); > > if (features & UFFD_FEATURE_WP_UNPOPULATED && test_type == TEST_ANON) > flags = UFFD_FEATURE_WP_UNPOPULATED; > @@ -324,7 +324,7 @@ static int userfaultfd_stress(void) > */ > uffd_test_ops->release_pages(area_dst); > > - uffd_stats_reset(args, nr_cpus); > + uffd_stats_reset(args, nr_threads); > > /* bounce pass */ > if (stress(args)) { > @@ -358,7 +358,7 @@ static int userfaultfd_stress(void) > > swap(area_src_alias, area_dst_alias); > > - uffd_stats_report(args, nr_cpus); > + uffd_stats_report(args, nr_threads); > } > uffd_test_ctx_clear(); > > @@ -452,9 +452,9 @@ int main(int argc, char **argv) > return KSFT_SKIP; > } > > - nr_cpus = sysconf(_SC_NPROCESSORS_ONLN); > + nr_threads = sysconf(_SC_NPROCESSORS_ONLN); > > - nr_pages_per_cpu = bytes / page_size / nr_cpus; > + nr_pages_per_cpu = bytes / page_size / nr_threads; > if (!nr_pages_per_cpu) { > _err("invalid MiB"); > usage(); > @@ -465,7 +465,7 @@ int main(int argc, char **argv) > _err("invalid bounces"); > usage(); > } > - nr_pages = nr_pages_per_cpu * nr_cpus; > + nr_pages = nr_pages_per_cpu * nr_threads; > > printf("nr_pages: %lu, nr_pages_per_cpu: %lu\n", > nr_pages, nr_pages_per_cpu); > diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c > index 9ff71fa1f9bf09b3ae599250663a25bbe2c13b8a..2f84fae5642c6f91b75fbf5f5d59ae64a1c15f92 100644 > --- a/tools/testing/selftests/mm/uffd-unit-tests.c > +++ b/tools/testing/selftests/mm/uffd-unit-tests.c > @@ -197,7 +197,7 @@ uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test, > > nr_pages = UFFD_TEST_MEM_SIZE / page_size; > /* TODO: remove this global var.. it's so ugly */ > - nr_cpus = 1; > + nr_threads = 1; > > /* Initialize test arguments */ > args->mem_type = mem_type; > Taking a cursory look at the test, it creates three threads for each cpu. The bounding of the variable is fine but that being the reason to rename the variable is not making sense to me.
On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote: > Taking a cursory look at the test, it creates three threads for each cpu. > The bounding of the variable is fine but that being the reason to rename the > variable is not making sense to me. Hmm yeah the name needs to be more abstract. Do you think nr_workers would be confusing? Or even just "parallelism" or nr_parallel? Or any other ideas? FWIW I briefly looked at just cleaning this up to remove the global variable but that's a bigger time investment than I can afford here I think. (The local variable in stress() would still need a better name anyway). Thanks for the review BTW!
On 03/03/25 3:17 pm, Brendan Jackman wrote: > On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote: >> Taking a cursory look at the test, it creates three threads for each cpu. >> The bounding of the variable is fine but that being the reason to rename the >> variable is not making sense to me. > > Hmm yeah the name needs to be more abstract. Do you think nr_workers > would be confusing? Or even just "parallelism" or nr_parallel? Or any > other ideas? > > FWIW I briefly looked at just cleaning this up to remove the global > variable but that's a bigger time investment than I can afford here I > think. (The local variable in stress() would still need a better name > anyway). > > Thanks for the review BTW! Your welcome. I personally prefer leaving it as is; unless someone comes up and completely cleans up the structure, let us save our collective brain cycles for more meaningful battles than renaming variables :)
On Mon, Mar 03, 2025 at 03:48:38PM +0530, Dev Jain wrote: > > > On 03/03/25 3:17 pm, Brendan Jackman wrote: > > On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote: > > > Taking a cursory look at the test, it creates three threads for each cpu. > > > The bounding of the variable is fine but that being the reason to rename the > > > variable is not making sense to me. > > > > Hmm yeah the name needs to be more abstract. Do you think nr_workers > > would be confusing? Or even just "parallelism" or nr_parallel? Or any > > other ideas? > > > > FWIW I briefly looked at just cleaning this up to remove the global > > variable but that's a bigger time investment than I can afford here I > > think. (The local variable in stress() would still need a better name > > anyway). > > > > Thanks for the review BTW! > > Your welcome. > > I personally prefer leaving it as is; unless someone comes up and completely > cleans up the structure, let us save our collective brain cycles for more > meaningful battles than renaming variables :) Hmm, I think that's a false economy on brain cycles. A variable called nr_cpus that isn't a number of CPUs is bound to waste a bunch of mental energy at some point in the future. Unless you strongly object I'll go for nr_parallel. It's not a great name but, well... I think that probably just suggests it's not a great variable, and I don't have time to fix that.
On 03/03/25 4:04 pm, Brendan Jackman wrote: > On Mon, Mar 03, 2025 at 03:48:38PM +0530, Dev Jain wrote: >> >> >> On 03/03/25 3:17 pm, Brendan Jackman wrote: >>> On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote: >>>> Taking a cursory look at the test, it creates three threads for each cpu. >>>> The bounding of the variable is fine but that being the reason to rename the >>>> variable is not making sense to me. >>> >>> Hmm yeah the name needs to be more abstract. Do you think nr_workers >>> would be confusing? Or even just "parallelism" or nr_parallel? Or any >>> other ideas? >>> >>> FWIW I briefly looked at just cleaning this up to remove the global >>> variable but that's a bigger time investment than I can afford here I >>> think. (The local variable in stress() would still need a better name >>> anyway). >>> >>> Thanks for the review BTW! >> >> Your welcome. >> >> I personally prefer leaving it as is; unless someone comes up and completely >> cleans up the structure, let us save our collective brain cycles for more >> meaningful battles than renaming variables :) > > Hmm, I think that's a false economy on brain cycles. A variable called > nr_cpus that isn't a number of CPUs is bound to waste a bunch of > mental energy at some point in the future. > > Unless you strongly object I'll go for nr_parallel. It's not a great > name but, well... I think that probably just suggests it's not a great > variable, and I don't have time to fix that. nr_parallel sounds better for sure. In case you send out a new patch: Reviewed-by: Dev Jain <dev.jain@arm.com>
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index 717539eddf98754250e70e564cd9a59f398bd7ea..a72a2ed5e89480ed06c81b034967ed5ae5f8cad5 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -10,7 +10,7 @@ #define BASE_PMD_ADDR ((void *)(1UL << 30)) volatile bool test_uffdio_copy_eexist = true; -unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; +unsigned long nr_threads, 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; @@ -269,7 +269,7 @@ void uffd_test_ctx_clear(void) size_t i; if (pipefd) { - for (i = 0; i < nr_cpus * 2; ++i) { + for (i = 0; i < nr_threads * 2; ++i) { if (close(pipefd[i])) err("close pipefd"); } @@ -365,10 +365,10 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg) */ uffd_test_ops->release_pages(area_dst); - pipefd = malloc(sizeof(int) * nr_cpus * 2); + pipefd = malloc(sizeof(int) * nr_threads * 2); if (!pipefd) err("pipefd"); - for (cpu = 0; cpu < nr_cpus; cpu++) + for (cpu = 0; cpu < nr_threads; cpu++) if (pipe2(&pipefd[cpu * 2], O_CLOEXEC | O_NONBLOCK)) err("pipe"); diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index a70ae10b5f6206daecc8e19ed3e3bbb388e265aa..604e3572fe17280ae346b031e2e867e039578f95 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -98,7 +98,7 @@ struct uffd_test_case_ops { }; typedef struct uffd_test_case_ops uffd_test_case_ops_t; -extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; +extern unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size; extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap; extern int uffd, uffd_flags, finished, *pipefd, test_type; extern bool map_shared; diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c index ed68436fac62c76e2ca7060c661487f2f8a6ab45..ec842bbb9f18e291fa51de0ed8d1fbf9aaf14372 100644 --- a/tools/testing/selftests/mm/uffd-stress.c +++ b/tools/testing/selftests/mm/uffd-stress.c @@ -179,12 +179,12 @@ static void *background_thread(void *arg) static int stress(struct uffd_args *args) { unsigned long cpu; - pthread_t locking_threads[nr_cpus]; - pthread_t uffd_threads[nr_cpus]; - pthread_t background_threads[nr_cpus]; + pthread_t locking_threads[nr_threads]; + pthread_t uffd_threads[nr_threads]; + pthread_t background_threads[nr_threads]; finished = 0; - for (cpu = 0; cpu < nr_cpus; cpu++) { + for (cpu = 0; cpu < nr_threads; cpu++) { if (pthread_create(&locking_threads[cpu], &attr, locking_thread, (void *)cpu)) return 1; @@ -202,7 +202,7 @@ static int stress(struct uffd_args *args) background_thread, (void *)cpu)) return 1; } - for (cpu = 0; cpu < nr_cpus; cpu++) + for (cpu = 0; cpu < nr_threads; cpu++) if (pthread_join(background_threads[cpu], NULL)) return 1; @@ -218,11 +218,11 @@ static int stress(struct uffd_args *args) uffd_test_ops->release_pages(area_src); finished = 1; - for (cpu = 0; cpu < nr_cpus; cpu++) + for (cpu = 0; cpu < nr_threads; cpu++) if (pthread_join(locking_threads[cpu], NULL)) return 1; - for (cpu = 0; cpu < nr_cpus; cpu++) { + for (cpu = 0; cpu < nr_threads; cpu++) { char c; if (bounces & BOUNCE_POLL) { if (write(pipefd[cpu*2+1], &c, 1) != 1) @@ -245,11 +245,11 @@ static int userfaultfd_stress(void) { void *area; unsigned long nr; - struct uffd_args args[nr_cpus]; + struct uffd_args args[nr_threads]; uint64_t mem_size = nr_pages * page_size; int flags = 0; - memset(args, 0, sizeof(struct uffd_args) * nr_cpus); + memset(args, 0, sizeof(struct uffd_args) * nr_threads); if (features & UFFD_FEATURE_WP_UNPOPULATED && test_type == TEST_ANON) flags = UFFD_FEATURE_WP_UNPOPULATED; @@ -324,7 +324,7 @@ static int userfaultfd_stress(void) */ uffd_test_ops->release_pages(area_dst); - uffd_stats_reset(args, nr_cpus); + uffd_stats_reset(args, nr_threads); /* bounce pass */ if (stress(args)) { @@ -358,7 +358,7 @@ static int userfaultfd_stress(void) swap(area_src_alias, area_dst_alias); - uffd_stats_report(args, nr_cpus); + uffd_stats_report(args, nr_threads); } uffd_test_ctx_clear(); @@ -452,9 +452,9 @@ int main(int argc, char **argv) return KSFT_SKIP; } - nr_cpus = sysconf(_SC_NPROCESSORS_ONLN); + nr_threads = sysconf(_SC_NPROCESSORS_ONLN); - nr_pages_per_cpu = bytes / page_size / nr_cpus; + nr_pages_per_cpu = bytes / page_size / nr_threads; if (!nr_pages_per_cpu) { _err("invalid MiB"); usage(); @@ -465,7 +465,7 @@ int main(int argc, char **argv) _err("invalid bounces"); usage(); } - nr_pages = nr_pages_per_cpu * nr_cpus; + nr_pages = nr_pages_per_cpu * nr_threads; printf("nr_pages: %lu, nr_pages_per_cpu: %lu\n", nr_pages, nr_pages_per_cpu); diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 9ff71fa1f9bf09b3ae599250663a25bbe2c13b8a..2f84fae5642c6f91b75fbf5f5d59ae64a1c15f92 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -197,7 +197,7 @@ uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test, nr_pages = UFFD_TEST_MEM_SIZE / page_size; /* TODO: remove this global var.. it's so ugly */ - nr_cpus = 1; + nr_threads = 1; /* Initialize test arguments */ args->mem_type = mem_type;
A later commit will bound this variable so it no longer necessarily matches the number of CPUs. Rename it appropriately. Signed-off-by: Brendan Jackman <jackmanb@google.com> --- tools/testing/selftests/mm/uffd-common.c | 8 ++++---- tools/testing/selftests/mm/uffd-common.h | 2 +- tools/testing/selftests/mm/uffd-stress.c | 28 ++++++++++++++-------------- tools/testing/selftests/mm/uffd-unit-tests.c | 2 +- 4 files changed, 20 insertions(+), 20 deletions(-)