* Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies
2021-09-13 12:30 [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies Aneesh Kumar K.V
@ 2021-09-13 14:24 ` Tyler Hicks
2021-09-13 15:10 ` Pasha Tatashin
2021-09-13 18:39 ` Zhansaya Bagdauletkyzy
2 siblings, 0 replies; 6+ messages in thread
From: Tyler Hicks @ 2021-09-13 14:24 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-mm, akpm, Zhansaya Bagdauletkyzy, Pavel Tatashin,
Hugh Dickins, Shuah Khan
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies
2021-09-13 12:30 [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies Aneesh Kumar K.V
2021-09-13 14:24 ` Tyler Hicks
@ 2021-09-13 15:10 ` Pasha Tatashin
2021-09-13 16:31 ` Aneesh Kumar K.V
2021-09-13 18:39 ` Zhansaya Bagdauletkyzy
2 siblings, 1 reply; 6+ messages in thread
From: Pasha Tatashin @ 2021-09-13 15:10 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-mm, Andrew Morton, Zhansaya Bagdauletkyzy, Tyler Hicks,
Hugh Dickins, Shuah Khan
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies
2021-09-13 15:10 ` Pasha Tatashin
@ 2021-09-13 16:31 ` Aneesh Kumar K.V
2021-09-13 18:36 ` Pasha Tatashin
0 siblings, 1 reply; 6+ messages in thread
From: Aneesh Kumar K.V @ 2021-09-13 16:31 UTC (permalink / raw)
To: Pasha Tatashin
Cc: linux-mm, Andrew Morton, Zhansaya Bagdauletkyzy, Tyler Hicks,
Hugh Dickins, Shuah Khan
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies
2021-09-13 16:31 ` Aneesh Kumar K.V
@ 2021-09-13 18:36 ` Pasha Tatashin
0 siblings, 0 replies; 6+ messages in thread
From: Pasha Tatashin @ 2021-09-13 18:36 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-mm, Andrew Morton, Zhansaya Bagdauletkyzy, Tyler Hicks,
Hugh Dickins, Shuah Khan
> >> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies
2021-09-13 12:30 [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies Aneesh Kumar K.V
2021-09-13 14:24 ` Tyler Hicks
2021-09-13 15:10 ` Pasha Tatashin
@ 2021-09-13 18:39 ` Zhansaya Bagdauletkyzy
2 siblings, 0 replies; 6+ messages in thread
From: Zhansaya Bagdauletkyzy @ 2021-09-13 18:39 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-mm, akpm, Pavel Tatashin, Tyler Hicks, Hugh Dickins, Shuah Khan
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread