| Message ID | 20210913123040.278031-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftest/vm: Fix ksm selftest to run with different NUMA topologies | expand |
On 2021-09-13 18:00:40, Aneesh Kumar K.V wrote: > Platforms can have non-contiguous NUMA nodes like below > > #numactl -H > available: 2 nodes (0,8) > ..... > node distances: > node 0 8 > 0: 10 40 > 8: 40 10 > > #numactl -H > available: 1 nodes (1) > .... > node distances: > node 1 > 1: 10 > > Hence update the test to not assume the presence of Node 0 and 1 > and also use numa_num_configured_nodes() instead of numa_max_node > for finding whether to skip the test. > I think this should be added: Fixes: 82e717ad3501 ("selftests: vm: add KSM merging across nodes test") The rest looks good to me: Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com> Tyler > Cc: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Hugh Dickins <hughd@google.com> > Cc: Shuah Khan <shuah@kernel.org> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > tools/testing/selftests/vm/ksm_tests.c | 32 +++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c > index b61dcdb44c5b..5bdfc5fe4d57 100644 > --- a/tools/testing/selftests/vm/ksm_tests.c > +++ b/tools/testing/selftests/vm/ksm_tests.c > @@ -354,12 +354,37 @@ static int check_ksm_zero_page_merge(int mapping, int prot, long page_count, int > return KSFT_FAIL; > } > > +static unsigned long get_next_mem_node(unsigned long node) > +{ > + > + long node_size; > + unsigned long i; > + unsigned long max_node = numa_max_node(); > + /* > + * start from node and find the next memory node > + */ > +restart: > + for (i = node + 1; i <= max_node; i++) { > + node_size = numa_node_size(i, NULL); > + if (node_size > 0) > + return i; > + } > + node = -1; > + goto restart; > +} > + > +static unsigned long get_first_mem_node(void) > +{ > + return get_next_mem_node(-1); > +} > + > static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_across_nodes, > size_t page_size) > { > void *numa1_map_ptr, *numa2_map_ptr; > struct timespec start_time; > int page_count = 2; > + unsigned long first_node; > > if (clock_gettime(CLOCK_MONOTONIC_RAW, &start_time)) { > perror("clock_gettime"); > @@ -370,7 +395,7 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > perror("NUMA support not enabled"); > return KSFT_SKIP; > } > - if (numa_max_node() < 1) { > + if (numa_num_configured_nodes() <= 1) { > printf("At least 2 NUMA nodes must be available\n"); > return KSFT_SKIP; > } > @@ -378,8 +403,9 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > return KSFT_FAIL; > > /* allocate 2 pages in 2 different NUMA nodes and fill them with the same data */ > - numa1_map_ptr = numa_alloc_onnode(page_size, 0); > - numa2_map_ptr = numa_alloc_onnode(page_size, 1); > + first_node = get_first_mem_node(); > + numa1_map_ptr = numa_alloc_onnode(page_size, first_node); > + numa2_map_ptr = numa_alloc_onnode(page_size, get_next_mem_node(first_node)); > if (!numa1_map_ptr || !numa2_map_ptr) { > perror("numa_alloc_onnode"); > return KSFT_FAIL; > -- > 2.31.1 >
On Mon, Sep 13, 2021 at 8:32 AM Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> wrote: > Hi Aneesh, Thank you for this fix. My comments below: > Platforms can have non-contiguous NUMA nodes like below Thanks for catching this. > +static unsigned long get_next_mem_node(unsigned long node) NUMA should be "int" not "unsigned long", especially as it is confusing with negative numbers used in get_first_mem_node(). Also, numa_max_node() returns int. > +{ > + > + long node_size; > + unsigned long i; > + unsigned long max_node = numa_max_node(); > + /* > + * start from node and find the next memory node > + */ > +restart: > + for (i = node + 1; i <= max_node; i++) { > + node_size = numa_node_size(i, NULL); > + if (node_size > 0) > + return i; > + } > + node = -1; > + goto restart; > +} I would rewrite the above without goto, and possibility of stacking in an infinite loop. Something like this should work: for (i = node + 1; i <= max_node + node; i++) { node_size = numa_node_size(i % (max_node + 1), NULL); if (node_size > 0) break; } return i % (max_node + 1); > + > +static unsigned long get_first_mem_node(void) > +{ > + return get_next_mem_node(-1); Next after the last node would make more sense: return get_next_mem_node(numa_max_node()); > +} > + > static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_across_nodes, > size_t page_size) > { > void *numa1_map_ptr, *numa2_map_ptr; > struct timespec start_time; > int page_count = 2; > + unsigned long first_node; Please use int. > > if (clock_gettime(CLOCK_MONOTONIC_RAW, &start_time)) { > perror("clock_gettime"); > @@ -370,7 +395,7 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > perror("NUMA support not enabled"); > return KSFT_SKIP; > } > - if (numa_max_node() < 1) { > + if (numa_num_configured_nodes() <= 1) { > printf("At least 2 NUMA nodes must be available\n"); > return KSFT_SKIP; > } > @@ -378,8 +403,9 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > return KSFT_FAIL; > > /* allocate 2 pages in 2 different NUMA nodes and fill them with the same data */ > - numa1_map_ptr = numa_alloc_onnode(page_size, 0); > - numa2_map_ptr = numa_alloc_onnode(page_size, 1); > + first_node = get_first_mem_node(); > + numa1_map_ptr = numa_alloc_onnode(page_size, first_node); > + numa2_map_ptr = numa_alloc_onnode(page_size, get_next_mem_node(first_node)); Thanks, Pasha
On 9/13/21 8:40 PM, Pasha Tatashin wrote: > On Mon, Sep 13, 2021 at 8:32 AM Aneesh Kumar K.V > <aneesh.kumar@linux.ibm.com> wrote: >> > > Hi Aneesh, > > Thank you for this fix. My comments below: > >> Platforms can have non-contiguous NUMA nodes like below > > Thanks for catching this. > >> +static unsigned long get_next_mem_node(unsigned long node) > > > > NUMA should be "int" not "unsigned long", especially as it is > confusing with negative numbers used in get_first_mem_node(). Also, > numa_max_node() returns int. > sure will update. >> +{ >> + >> + long node_size; >> + unsigned long i; >> + unsigned long max_node = numa_max_node(); >> + /* >> + * start from node and find the next memory node >> + */ >> +restart: >> + for (i = node + 1; i <= max_node; i++) { >> + node_size = numa_node_size(i, NULL); >> + if (node_size > 0) >> + return i; >> + } >> + node = -1; >> + goto restart; >> +} > > I would rewrite the above without goto, and possibility of stacking in > an infinite loop. Something like this should work: > > for (i = node + 1; i <= max_node + node; i++) { > node_size = numa_node_size(i % (max_node + 1), NULL); > if (node_size > 0) > break; > } > > return i % (max_node + 1); > I didn't quiet follow this. not all nodes can have memory and node numbers are discontiguous. >> + >> +static unsigned long get_first_mem_node(void) >> +{ >> + return get_next_mem_node(-1); > > Next after the last node would make more sense: > > return get_next_mem_node(numa_max_node()); > > Yes, that would work, but is that really useful? We would essentially skip the for loop in first iteration set node = -1 internally and do the for loop again. >> +} >> + >> static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_across_nodes, >> size_t page_size) >> { >> void *numa1_map_ptr, *numa2_map_ptr; >> struct timespec start_time; >> int page_count = 2; >> + unsigned long first_node; > > Please use int. > >> >> if (clock_gettime(CLOCK_MONOTONIC_RAW, &start_time)) { >> perror("clock_gettime"); >> @@ -370,7 +395,7 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a >> perror("NUMA support not enabled"); >> return KSFT_SKIP; >> } >> - if (numa_max_node() < 1) { >> + if (numa_num_configured_nodes() <= 1) { >> printf("At least 2 NUMA nodes must be available\n"); >> return KSFT_SKIP; >> } >> @@ -378,8 +403,9 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a >> return KSFT_FAIL; >> >> /* allocate 2 pages in 2 different NUMA nodes and fill them with the same data */ >> - numa1_map_ptr = numa_alloc_onnode(page_size, 0); >> - numa2_map_ptr = numa_alloc_onnode(page_size, 1); >> + first_node = get_first_mem_node(); >> + numa1_map_ptr = numa_alloc_onnode(page_size, first_node); >> + numa2_map_ptr = numa_alloc_onnode(page_size, get_next_mem_node(first_node)); > > Thanks, > Pasha > -aneesh
> >> + long node_size; > >> + unsigned long i; > >> + unsigned long max_node = numa_max_node(); > >> + /* > >> + * start from node and find the next memory node > >> + */ > >> +restart: > >> + for (i = node + 1; i <= max_node; i++) { > >> + node_size = numa_node_size(i, NULL); > >> + if (node_size > 0) > >> + return i; > >> + } > >> + node = -1; > >> + goto restart; > >> +} > > > > I would rewrite the above without goto, and possibility of stacking in > > an infinite loop. Something like this should work: > > > > for (i = node + 1; i <= max_node + node; i++) { > > node_size = numa_node_size(i % (max_node + 1), NULL); > > if (node_size > 0) > > break; > > } > > > > return i % (max_node + 1); > > > > > I didn't quiet follow this. not all nodes can have memory and node > numbers are discontiguous. I understand, the above loop searches for the first node with memory, and returns it. It cannot end up in the infinite loop in case if there are bugs where numa_node_size() are zero for all nodes for example. Also, it does not have gotos. > > >> + > >> +static unsigned long get_first_mem_node(void) > >> +{ > >> + return get_next_mem_node(-1); > > > > Next after the last node would make more sense: > > > > return get_next_mem_node(numa_max_node()); > > > > > > Yes, that would work, but is that really useful? We would essentially > skip the for loop in first iteration set node = -1 internally and do the > for loop again. It is useful in terms that get_next_mem_node() is supplied with real nodes. Also, it works better with the version of loop that I am proposing above: "max_node + node" when node == -1 does not yield the desired result. Thanks, Pasha
On Mon, Sep 13, 2021 at 06:00:40PM +0530, Aneesh Kumar K.V wrote: > Platforms can have non-contiguous NUMA nodes like below > Hi Aneesh, Thank you for pointing this out. Everything looks good to me: Reviewed-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com> Zhansaya > #numactl -H > available: 2 nodes (0,8) > ..... > node distances: > node 0 8 > 0: 10 40 > 8: 40 10 > > #numactl -H > available: 1 nodes (1) > .... > node distances: > node 1 > 1: 10 > > Hence update the test to not assume the presence of Node 0 and 1 > and also use numa_num_configured_nodes() instead of numa_max_node > for finding whether to skip the test. > > Cc: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Hugh Dickins <hughd@google.com> > Cc: Shuah Khan <shuah@kernel.org> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > tools/testing/selftests/vm/ksm_tests.c | 32 +++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c > index b61dcdb44c5b..5bdfc5fe4d57 100644 > --- a/tools/testing/selftests/vm/ksm_tests.c > +++ b/tools/testing/selftests/vm/ksm_tests.c > @@ -354,12 +354,37 @@ static int check_ksm_zero_page_merge(int mapping, int prot, long page_count, int > return KSFT_FAIL; > } > > +static unsigned long get_next_mem_node(unsigned long node) > +{ > + > + long node_size; > + unsigned long i; > + unsigned long max_node = numa_max_node(); > + /* > + * start from node and find the next memory node > + */ > +restart: > + for (i = node + 1; i <= max_node; i++) { > + node_size = numa_node_size(i, NULL); > + if (node_size > 0) > + return i; > + } > + node = -1; > + goto restart; > +} > + > +static unsigned long get_first_mem_node(void) > +{ > + return get_next_mem_node(-1); > +} > + > static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_across_nodes, > size_t page_size) > { > void *numa1_map_ptr, *numa2_map_ptr; > struct timespec start_time; > int page_count = 2; > + unsigned long first_node; > > if (clock_gettime(CLOCK_MONOTONIC_RAW, &start_time)) { > perror("clock_gettime"); > @@ -370,7 +395,7 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > perror("NUMA support not enabled"); > return KSFT_SKIP; > } > - if (numa_max_node() < 1) { > + if (numa_num_configured_nodes() <= 1) { > printf("At least 2 NUMA nodes must be available\n"); > return KSFT_SKIP; > } > @@ -378,8 +403,9 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a > return KSFT_FAIL; > > /* allocate 2 pages in 2 different NUMA nodes and fill them with the same data */ > - numa1_map_ptr = numa_alloc_onnode(page_size, 0); > - numa2_map_ptr = numa_alloc_onnode(page_size, 1); > + first_node = get_first_mem_node(); > + numa1_map_ptr = numa_alloc_onnode(page_size, first_node); > + numa2_map_ptr = numa_alloc_onnode(page_size, get_next_mem_node(first_node)); > if (!numa1_map_ptr || !numa2_map_ptr) { > perror("numa_alloc_onnode"); > return KSFT_FAIL; > -- > 2.31.1 >
diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c index b61dcdb44c5b..5bdfc5fe4d57 100644 --- a/tools/testing/selftests/vm/ksm_tests.c +++ b/tools/testing/selftests/vm/ksm_tests.c @@ -354,12 +354,37 @@ static int check_ksm_zero_page_merge(int mapping, int prot, long page_count, int return KSFT_FAIL; } +static unsigned long get_next_mem_node(unsigned long node) +{ + + long node_size; + unsigned long i; + unsigned long max_node = numa_max_node(); + /* + * start from node and find the next memory node + */ +restart: + for (i = node + 1; i <= max_node; i++) { + node_size = numa_node_size(i, NULL); + if (node_size > 0) + return i; + } + node = -1; + goto restart; +} + +static unsigned long get_first_mem_node(void) +{ + return get_next_mem_node(-1); +} + static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_across_nodes, size_t page_size) { void *numa1_map_ptr, *numa2_map_ptr; struct timespec start_time; int page_count = 2; + unsigned long first_node; if (clock_gettime(CLOCK_MONOTONIC_RAW, &start_time)) { perror("clock_gettime"); @@ -370,7 +395,7 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a perror("NUMA support not enabled"); return KSFT_SKIP; } - if (numa_max_node() < 1) { + if (numa_num_configured_nodes() <= 1) { printf("At least 2 NUMA nodes must be available\n"); return KSFT_SKIP; } @@ -378,8 +403,9 @@ static int check_ksm_numa_merge(int mapping, int prot, int timeout, bool merge_a return KSFT_FAIL; /* allocate 2 pages in 2 different NUMA nodes and fill them with the same data */ - numa1_map_ptr = numa_alloc_onnode(page_size, 0); - numa2_map_ptr = numa_alloc_onnode(page_size, 1); + first_node = get_first_mem_node(); + numa1_map_ptr = numa_alloc_onnode(page_size, first_node); + numa2_map_ptr = numa_alloc_onnode(page_size, get_next_mem_node(first_node)); if (!numa1_map_ptr || !numa2_map_ptr) { perror("numa_alloc_onnode"); return KSFT_FAIL;
Platforms can have non-contiguous NUMA nodes like below #numactl -H available: 2 nodes (0,8) ..... node distances: node 0 8 0: 10 40 8: 40 10 #numactl -H available: 1 nodes (1) .... node distances: node 1 1: 10 Hence update the test to not assume the presence of Node 0 and 1 and also use numa_num_configured_nodes() instead of numa_max_node for finding whether to skip the test. Cc: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> Cc: Tyler Hicks <tyhicks@linux.microsoft.com> Cc: Hugh Dickins <hughd@google.com> Cc: Shuah Khan <shuah@kernel.org> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- tools/testing/selftests/vm/ksm_tests.c | 32 +++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)