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

[Jack Steiner <steiner@sgi.com>]

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