linux-kernel - Re: [PATCH x86/mm UPDATED] x86-64, NUMA: Fix distance table handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110311082938.GC13038@htj.dyndns.org>
Date:	Fri, 11 Mar 2011 09:29:38 +0100
From:	Tejun Heo <tj@...nel.org>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	David Rientjes <rientjes@...gle.com>, Ingo Molnar <mingo@...e.hu>,
	tglx@...utronix.de, "H. Peter Anvin" <hpa@...or.com>,
	linux-kernel@...r.kernel.org
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/