diff mbox series

selftest/vm: Fix ksm selftest to run with different NUMA topologies

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

Commit Message

Aneesh Kumar K.V Sept. 13, 2021, 12:30 p.m. UTC
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(-)

Comments

Tyler Hicks Sept. 13, 2021, 2:24 p.m. UTC | #1
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
>
Pasha Tatashin Sept. 13, 2021, 3:10 p.m. UTC | #2
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
Aneesh Kumar K.V Sept. 13, 2021, 4:31 p.m. UTC | #3
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
Pasha Tatashin Sept. 13, 2021, 6:36 p.m. UTC | #4
> >> +       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
Zhansaya Bagdauletkyzy Sept. 13, 2021, 6:39 p.m. UTC | #5
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 mbox series

Patch

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;