Re: [PATCH x86/mm UPDATED] x86-64, NUMA: Fix distance tablehandling

[Tejun Heo]

Hey, Yinghai.

On Thu, Mar 10, 2011 at 10:46:55AM -0800, Yinghai Lu wrote:
> that will duplicate the code:
> 
>         nodes_parsed = numa_nodes_parsed;
>         numa_nodemask_from_meminfo(&nodes_parsed, &numa_meminfo);
> 
>         for_each_node_mask(i, nodes_parsed)
>                 cnt = i;
>         cnt++;
> 
> in numa_alloc_distance().
> 
> Why just let numa_alloc_distance() to be called at first and return cnt ?

Look at your patch.  It exports numa_alloc_distanc() and calls the
function explicitly just to use for (i...) loop instead of
for_each_node_mask().

If you look at how numa_set_distance() is used, it's supposed to be
fire-and-forget thing.  Specific NUMA configuration implementations
provide the information to the generic code but they don't have to
care how it's managed or whether something fails for whatever reason
and I'd like to keep it that way.  It's a small distinction but small
abstractions and simplifications like that add up to make the code
base easier to understand and maintain.  And you wanna break that for
what?  for (i...) instead of for_each_node_mask().

Also, I don't think your patch is correct.  Even if there is phys_dist
table, emu distance tables should be built because emulated nids don't
map to physical nids one to one.  ie. Two different emulated nids can
share a physical node and the distance table should explicitly reflect
that.

Nobody cares if we spend a bit more cycles going over numa bitmap some
more times during boot for frigging NUMA emulation.  It doesn't matter
AT ALL.  Don't micro optimize where it doesn't matter at the cost of
code readability and maintainability.  It is actively harmful.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/