Re: [PATCH 7/12] hugetlb: add per node hstate attributes

[David Rientjes <rientjes@google.com>]

On Fri, 9 Oct 2009, Lee Schermerhorn wrote:

> > > +static void hugetlb_register_all_nodes(void)
> > > +{
> > > +	int nid;
> > > +
> > > +	for (nid = 0; nid < nr_node_ids; nid++) {
> > > +		struct node *node = &node_devices[nid];
> > > +		if (node->sysdev.id == nid)
> > > +			hugetlb_register_node(node);
> > > +	}
> > 
> > This looks like another use of for_each_node_mask over N_HIGH_MEMORY.  I 
> > previously asked if the check for node->sysdev.id == nid is still 
> > necessary at this point?
> 
> 
> Sorry.  The check for sysdev.id == nid is there to ensure that this node
> sysdev has been registered when this function is called.  nr_node_ids is
> the maximum node id seen so far, but we can't assume that all nodes
> 0..nr_node_ids are present/on-line.  
> 
> As for using for_each_node_mask:  I think that would be OK.  This code
> works because hugetlb_register_node() filters out nodes w/o memory; so
> only visiting nodes with memory should work as well.  We can change this
> [for consistency] with an incremental patch, if you like.  
> 
> I'd hate to respin V11 for just this.  But, if we have to for other
> reasons, I'll [try to remember to] do this.
> 

I don't think it's necessary for a v11, I'd like to see this patchset 
(perhaps minus patch 12/12 until we figure out whether it's actually 
needed or not) added to -mm and then work on it there.  This particular 
case is only a small cleanup, but my curiosity really laid more in why 
node->sysdev.id == nid was necessary instead of simply using 
for_each_node_mask(nid, node_states[N_HIGH_MEMORY]) since that should 
certainly be a subset of for_each_online_node(nid).

Thanks for the clarification, we can do an incremental patch on -mm once 
it's merged.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>