[PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Dean Roe]

Fix a for-loop in sn_hwperf_geoid_to_cnode().  It needs to loop over
num_cnodes to ensure it can still process TIO nodes in addition to
compute nodes on systems with many nodes.

	Signed-off-by: Dean Roe <roe@sgi.com>


Index: linux-2.6/arch/ia64/sn/kernel/sn2/sn_hwperf.c
=================================--- linux-2.6.orig/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ linux-2.6/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -110,7 +110,7 @@
 	if (sn_hwperf_location_to_bpos(location, &rack, &bay, &slot, &slab))
 		return -1;
 
-	for_each_node(cnode) {
+	for (cnode = 0; cnode < num_cnodes; cnode++) {
 		geoid = cnodeid_get_geoid(cnode);
 		module_id = geo_module(geoid);
 		this_rack = MODULE_GET_RACK(module_id);

RE: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Luck, Tony]

-	for_each_node(cnode) {
+	for (cnode = 0; cnode < num_cnodes; cnode++) {

Do we really need to fix "for_each_node()" ... having a macro
named this way that doesn't actually loop over all nodes looks
like an invitation to get things wrong in the future.

-Tony

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Jack Steiner]

On Fri, Mar 03, 2006 at 09:01:57AM -0800, Luck, Tony wrote:
> -	for_each_node(cnode) {
> +	for (cnode = 0; cnode < num_cnodes; cnode++) {
> 
> Do we really need to fix "for_each_node()" ... having a macro
> named this way that doesn't actually loop over all nodes looks
> like an invitation to get things wrong in the future.

I don't think that will work in the short term. Once we
get to ACPI3.0, yes.

The problem is that SN & the generic kernel are not in agreement 
on the definition of a "node". There are several problems.

ACPI2.0 limits the total number of nodes to 256 (PXM), SN needs 
more.  

In addition, it is difficult to describe IO-only nodes to the 
generic kernel.  We would need SRAT entries for IO-only nodes 
but those tables don't currently exist. Even if they did, we still 
have the 256 node limit.

As an interim & admittedly "hackey" solution, we have introduced the
notion of "cnodes". For cpu & memory nodes, the cnode number is
the same as the nid (generic kernel node number). IO nodes are
guaranteed to have numbers assigned after the last nid. IO nodes
may have number >= 256.

Only SN code is aware of cnodes. For example, IO cnodes are not in any
of the node maps: node_online_map, node_possible_map, etc.
IO cnodes don't have many (any?) of the usual per-node tables. 



Perhaps for consistency, we should add an SN macro:

	for_each_cnode(cnode)
	     or
	for_each_sn_cnode(cnode)	# to indicate this is SN only


All of these ugly hacks will go away when we get to ACPI3.0.

RE: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Luck, Tony]

>As an interim & admittedly "hackey" solution, we have introduced the
>notion of "cnodes". For cpu & memory nodes, the cnode number is
>the same as the nid (generic kernel node number). IO nodes are
>guaranteed to have numbers assigned after the last nid. IO nodes
>may have number >= 256.

Right now there are only three uses of for_each_node().  The two
in mm/slab.c only need to worry about nodes that have memory, so from
your description above, they won't be at all concerned about the I/O
only nodes that they will fail to scan.

Any ideas on the relative proportions of nodes with/without memory?
Should we consider renaming "for_each_node()" as "for_each_mnode()"
(meaning only scan nodes with memory).  That would stop mm/slab.c
from allocating useless arraycaches for nodes with no memory.  But
this is turning into a discussion that would have to happen on LKML.

Perhaps you could add an /*ACPI3.0-FIXME*/ comment (or some such)
to the loop in Dean's patch as a reminder to fix this later?  That
might also serve as a clue to any janitor that tries to clean
up this code back to using "for_each_node()" (and a reminder to
me to not take such a patch).

-Tony

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Dean Roe]

On Fri, Mar 03, 2006 at 01:52:56PM -0800, Luck, Tony wrote:
> Any ideas on the relative proportions of nodes with/without memory?
> Should we consider renaming "for_each_node()" as "for_each_mnode()"
> (meaning only scan nodes with memory).  That would stop mm/slab.c
> from allocating useless arraycaches for nodes with no memory.  But
> this is turning into a discussion that would have to happen on LKML.

Memory nodes are far more prevalent than IO nodes.  I don't think it
is worth changing this, at least not at this time.


> 
> Perhaps you could add an /*ACPI3.0-FIXME*/ comment (or some such)
> to the loop in Dean's patch as a reminder to fix this later?  That
> might also serve as a clue to any janitor that tries to clean
> up this code back to using "for_each_node()" (and a reminder to
> me to not take such a patch).

I'll add a comment and send out a new patch in a few minutes.

Thanks,
Dean

[PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Dean Roe]

Patch attempt #2 with a comment added.  Fix a for-loop in
sn_hwperf_geoid_to_cnode().  It needs to loop over num_cnodes to
ensure it can still process TIO nodes in addition to compute nodes
on systems with many nodes.

	Signed-off-by: Dean Roe <roe@sgi.com>


Index: linux-2.6/arch/ia64/sn/kernel/sn2/sn_hwperf.c
=================================--- linux-2.6.orig/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ linux-2.6/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -110,7 +110,11 @@
 	if (sn_hwperf_location_to_bpos(location, &rack, &bay, &slot, &slab))
 		return -1;
 
-	for_each_node(cnode) {
+	/*
+	 * FIXME: replace with cleaner for_each_XXX macro which addresses
+	 * both compute and IO nodes once ACPI3.0 is available.
+	 */
+	for (cnode = 0; cnode < num_cnodes; cnode++) {
 		geoid = cnodeid_get_geoid(cnode);
 		module_id = geo_module(geoid);
 		this_rack = MODULE_GET_RACK(module_id);

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Bjorn Helgaas]

On Monday 06 March 2006 09:32, Dean Roe wrote:
> -	for_each_node(cnode) {
> +	/*
> +	 * FIXME: replace with cleaner for_each_XXX macro which addresses
> +	 * both compute and IO nodes once ACPI3.0 is available.
> +	 */
> +	for (cnode = 0; cnode < num_cnodes; cnode++) {

I don't understand this ACPI 3.0 dependency.  Can't you just define
for_each_XXX() the way you want it, and fill in the bitmask or whatever
it uses either (a) using ACPI 3.0 data, or (b) some interim hack?

Bjorn

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Dean Roe]

On Mon, Mar 06, 2006 at 10:35:19AM -0700, Bjorn Helgaas wrote:
> On Monday 06 March 2006 09:32, Dean Roe wrote:
> > -	for_each_node(cnode) {
> > +	/*
> > +	 * FIXME: replace with cleaner for_each_XXX macro which addresses
> > +	 * both compute and IO nodes once ACPI3.0 is available.
> > +	 */
> > +	for (cnode = 0; cnode < num_cnodes; cnode++) {
> 
> I don't understand this ACPI 3.0 dependency.  Can't you just define
> for_each_XXX() the way you want it, and fill in the bitmask or whatever
> it uses either (a) using ACPI 3.0 data, or (b) some interim hack?
> 
> Bjorn

I can't really tell from your response, so...did you see Jack's explanation
of this?
    http://marc.theaimsgroup.com/?l=linux-ia64&m\x114141537904761&w=2

Are you saying you *really* want a for_each_sn_cnode() macro?  I guess
we can go that route if necessary...I just prefer the one-line change
rather than changing 4-5 files when we aren't really sure yet what the
final implementation will look like.  I just want to make sure I am reading
you correctly.

Thanks,
Dean

-- 
Dean Roe
Silicon Graphics, Inc.
roe@sgi.com

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Bjorn Helgaas]

On Wednesday 08 March 2006 15:02, Dean Roe wrote:
> On Mon, Mar 06, 2006 at 10:35:19AM -0700, Bjorn Helgaas wrote:
> > On Monday 06 March 2006 09:32, Dean Roe wrote:
> > > -	for_each_node(cnode) {
> > > +	/*
> > > +	 * FIXME: replace with cleaner for_each_XXX macro which addresses
> > > +	 * both compute and IO nodes once ACPI3.0 is available.
> > > +	 */
> > > +	for (cnode = 0; cnode < num_cnodes; cnode++) {
> > 
> > I don't understand this ACPI 3.0 dependency.  Can't you just define
> > for_each_XXX() the way you want it, and fill in the bitmask or whatever
> > it uses either (a) using ACPI 3.0 data, or (b) some interim hack?
> > 
> > Bjorn
> 
> I can't really tell from your response, so...did you see Jack's explanation
> of this?
>     http://marc.theaimsgroup.com/?l=linux-ia64&m\x114141537904761&w=2
> 
> Are you saying you *really* want a for_each_sn_cnode() macro?  I guess
> we can go that route if necessary...I just prefer the one-line change
> rather than changing 4-5 files when we aren't really sure yet what the
> final implementation will look like.

I saw his response, but it wasn't clear to me what ACPI 3.0 is going
to solve.  Are you saying that with ACPI 3.0, you will be able to
use for_each_node()?

If that's the case, my question is, why can't you use for_each_node()
today, and use some interim hack to fill in node_possible_map?

If not, what "for_each_XXX" macro are you planning to use when
you have ACPI 3.0?

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Robin Holt]

On Wed, Mar 08, 2006 at 04:55:45PM -0700, Bjorn Helgaas wrote:
> On Wednesday 08 March 2006 15:02, Dean Roe wrote:
> > On Mon, Mar 06, 2006 at 10:35:19AM -0700, Bjorn Helgaas wrote:
> > > On Monday 06 March 2006 09:32, Dean Roe wrote:
> > > > -	for_each_node(cnode) {
> > > > +	/*
> > > > +	 * FIXME: replace with cleaner for_each_XXX macro which addresses
> > > > +	 * both compute and IO nodes once ACPI3.0 is available.
> > > > +	 */
> > > > +	for (cnode = 0; cnode < num_cnodes; cnode++) {
> > > 
> > > I don't understand this ACPI 3.0 dependency.  Can't you just define
> > > for_each_XXX() the way you want it, and fill in the bitmask or whatever
> > > it uses either (a) using ACPI 3.0 data, or (b) some interim hack?
> > > 
> > > Bjorn
> > 
> > I can't really tell from your response, so...did you see Jack's explanation
> > of this?
> >     http://marc.theaimsgroup.com/?l=linux-ia64&m\x114141537904761&w=2
> > 
> > Are you saying you *really* want a for_each_sn_cnode() macro?  I guess
> > we can go that route if necessary...I just prefer the one-line change
> > rather than changing 4-5 files when we aren't really sure yet what the
> > final implementation will look like.
> 
> I saw his response, but it wasn't clear to me what ACPI 3.0 is going
> to solve.  Are you saying that with ACPI 3.0, you will be able to
> use for_each_node()?

The node informatoin is stored in a single byte.  With the introduction
of I/O only nodes, we can easily exceed the 512 node limit placed on
the byte size.  ACPI 3.0 should raise that node limit to at least a
16-bit word.

> 
> If that's the case, my question is, why can't you use for_each_node()
> today, and use some interim hack to fill in node_possible_map?

The node field size does not allow for it.

> If not, what "for_each_XXX" macro are you planning to use when
> you have ACPI 3.0?

Not sure yet because ACPI 3.0 is still way off in the future.  We
will know more when that time comes.

Robin

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Bjorn Helgaas]

On Thursday 09 March 2006 09:12, Robin Holt wrote:
> On Wed, Mar 08, 2006 at 04:55:45PM -0700, Bjorn Helgaas wrote:
> > On Wednesday 08 March 2006 15:02, Dean Roe wrote:
> > > Are you saying you *really* want a for_each_sn_cnode() macro?  I guess
> > > we can go that route if necessary...I just prefer the one-line change
> > > rather than changing 4-5 files when we aren't really sure yet what the
> > > final implementation will look like.
> > 
> > I saw his response, but it wasn't clear to me what ACPI 3.0 is going
> > to solve.  Are you saying that with ACPI 3.0, you will be able to
> > use for_each_node()?
> 
> The node informatoin is stored in a single byte.  With the introduction
> of I/O only nodes, we can easily exceed the 512 node limit placed on
> the byte size.  ACPI 3.0 should raise that node limit to at least a
> 16-bit word.

I'm only trying to point out that you should be able to separate the
for_each_XXX() interface from the current ACPI limit.

> > If that's the case, my question is, why can't you use for_each_node()
> > today, and use some interim hack to fill in node_possible_map?
> 
> The node field size does not allow for it.

Which node field doesn't allow it?  for_each_node() uses the
node_possible_mask bitmap, which can be any size you need.

> > If not, what "for_each_XXX" macro are you planning to use when
> > you have ACPI 3.0?
> 
> Not sure yet because ACPI 3.0 is still way off in the future.  We
> will know more when that time comes.

If you think for_each_node() is the correct interface to use here,
just use it, and make node_possible_mask large enough.  That means
you might need some glue code deal with the fact that ACPI 2.0
can't populate the whole thing, but that's not a problem.

If you think you need something other than for_each_node(), why
not just take a stab at defining the correct interface now, and
fix it later if you need to?  I think that's better than putting
in something that you *know* will need to be changed later.

Re: [PATCH] fix for-loop in sn_hwperf_geoid_to_cnode()

[Jack Steiner]

On Thu, Mar 09, 2006 at 10:57:09AM -0700, Bjorn Helgaas wrote:
> On Thursday 09 March 2006 09:12, Robin Holt wrote:
> > On Wed, Mar 08, 2006 at 04:55:45PM -0700, Bjorn Helgaas wrote:
> > > On Wednesday 08 March 2006 15:02, Dean Roe wrote:
> > > > Are you saying you *really* want a for_each_sn_cnode() macro?  I guess
> > > > we can go that route if necessary...I just prefer the one-line change
> > > > rather than changing 4-5 files when we aren't really sure yet what the
> > > > final implementation will look like.
> > > 
> > > I saw his response, but it wasn't clear to me what ACPI 3.0 is going
> > > to solve.  Are you saying that with ACPI 3.0, you will be able to
> > > use for_each_node()?
> > 
> > The node informatoin is stored in a single byte.  With the introduction
> > of I/O only nodes, we can easily exceed the 512 node limit placed on
> > the byte size.  ACPI 3.0 should raise that node limit to at least a
> > 16-bit word.
> 
> I'm only trying to point out that you should be able to separate the
> for_each_XXX() interface from the current ACPI limit.
> 
> > > If that's the case, my question is, why can't you use for_each_node()
> > > today, and use some interim hack to fill in node_possible_map?
> > 
> > The node field size does not allow for it.
> 
> Which node field doesn't allow it?  for_each_node() uses the
> node_possible_mask bitmap, which can be any size you need.

If we extend node_possible_map, I think we would also have to extend several
other tables as well. For example, it is possibly that code that finds
a node in the node_possible_map may also expect to find the node in
the slit, node_to_cpu_mask, nid_to_pxm_map, ...

All of this could be done but it requires a lot more code & testing than
Deans's patch and it would still be a hack that would be deleted later when
we get the _real_ solution that involve BIOS & ACPI changes. I'd rather
minimize any new temporary hacks & focus on the longer term solution. 

In fact, I posted part of what you are suggesting last week:
	http://marc.theaimsgroup.com/?l=linux-ia64&m\x114133701316260&w=2

This patch extends the size of the node_possible_map. The patch adds an
option to support up to 1024 cpu/memory nodes on SN systems. The patch does
not yet address the problem of IO nodes. For that, we need ACPI3.0 and
a new BIOS. The patch, by necessity, is also SN specific until we get
to ACPI3.0.

This patch has not yet been accepted - hopefully a candidate for 2.6.17.
However, this is the first step in the direction that you are suggesting.

In the interim, we need a fix in 2.6.16 (we have problems on our large systems
without it) & I think Dean's patch should be ok.  It is a 1-line fix, 
is used in only one spot & is not likely to be copied to other places.


> 
> > > If not, what "for_each_XXX" macro are you planning to use when
> > > you have ACPI 3.0?
> > 
> > Not sure yet because ACPI 3.0 is still way off in the future.  We
> > will know more when that time comes.
> 
> If you think for_each_node() is the correct interface to use here,
> just use it, and make node_possible_mask large enough.  That means
> you might need some glue code deal with the fact that ACPI 2.0
> can't populate the whole thing, but that's not a problem.
> 
> If you think you need something other than for_each_node(), why
> not just take a stab at defining the correct interface now, and
> fix it later if you need to?  I think that's better than putting
> in something that you *know* will need to be changed later.

I expect that once IO nodes are fully supported by ACPI, we will use the
standard "for_each_node" macros. It's possible that we may want to
introduce a "for_each_node_with_memory" macro (there is already a
"for_each_node_with_cpus" macro). A lot more design is required...


--- Jack