Re: [PATCH] selftest/vm: Fix ksm selftest to run with different NUMA topologies

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

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