Messages in this thread | | | Date | Fri, 11 Mar 2011 09:29:38 +0100 | From | Tejun Heo <> | Subject | Re: [PATCH x86/mm UPDATED] x86-64, NUMA: Fix distance table handling |
| |
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
| |