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

"Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> · Mon, 13 Sep 2021 22:01:27 +0530




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@xxxxxxxxxxxxx> 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







