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

[Pasha Tatashin <pasha.tatashin@soleen.com>]

> >> +       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