Re: [PATCH 1/5] sched/topology: Introduce for_each_numa_node() iterator

[Yury Norov <yury.norov@gmail.com>]

On Fri, Feb 07, 2025 at 04:44:07PM +0100, Andrea Righi wrote:
> Hi Yury,
> 
> On Thu, Feb 06, 2025 at 10:57:19PM -0500, Yury Norov wrote:
> > On Thu, Feb 06, 2025 at 09:15:31PM +0100, Andrea Righi wrote:
> ...
> > > @@ -261,6 +267,29 @@ sched_numa_hop_mask(unsigned int node, unsigned int hops)
> > >  }
> > >  #endif	/* CONFIG_NUMA */
> > >  
> > > +/**
> > > + * for_each_numa_node - iterate over NUMA nodes at increasing hop distances
> > > + *                      from a given starting node.
> > > + * @node: the iteration variable, representing the current NUMA node.
> > > + * @start: the NUMA node to start the iteration from.
> > > + * @visited: a nodemask_t to track the visited nodes.
> > 
> > nit: s/nodemask_t/nodemask
> 
> The type is actually nodemask_t, do you think it's better to mention
> nodemask instead?

We just don't put types in variables descriptions. Refer the comment
on top of memcpy():

 /**
  * memcpy - Copy one area of memory to another
  * @dest: Where to copy to
  * @src: Where to copy from
  * @count: The size of the area.
  *
  * You should not use this function to access IO space, use memcpy_toio()
  * or memcpy_fromio() instead.
  */
 void *memcpy(void *dest, const void *src, size_t count)

We don't say like
  * @count: The size_t of the area.

Right?
 
> > int numa_nearest_nodemask(int node, const nodemask_t *mask);
> > #define for_each_numa_node(node, unvisited)			                      \
> > 	for (int start = (node), n = numa_nearest_nodemask(start, &(unvisited));    \
> > 	     n < MAX_NUMNODES;					                      \
> >              node_clear(n, (visited)), n = numa_nearest_nodemask(start, &(visited)))
> 
> I like the numa_nearest_nodemask() idea, I'll do some experiemnts with it.

Yeah, this one looks like the most generic API I can figure out, and
still it fits your needs just as well.

Please in the comment refer that the 'unvisited' will be touched by
the macro.