On Thu, 2009-10-08 at 13:42 -0700, David Rientjes wrote: > On Thu, 8 Oct 2009, Lee Schermerhorn wrote: > <snip> > > +static struct attribute_group per_node_hstate_attr_group = { > > + .attrs = per_node_hstate_attrs, > > +}; > > + > > +/* > > + * kobj_to_node_hstate - lookup global hstate for node sysdev hstate attr kobj. > > + * Returns node id via non-NULL nidp. > > + */ > > +static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp) > > +{ > > + int nid; > > + > > + for (nid = 0; nid < nr_node_ids; nid++) { > > I previously asked if this should use for_each_node_mask() instead? sorry, missed this comment [and one at end] in my prev response. Too much multi-tasking. This also could interate over a node mask for consistency, I think. Again, current version works because we're looking for node sysdev based on a per node attribute kobj. We only add the attributes to nodes with memory. So, we're potentially visiting a few more nodes than necessary on some platforms. Shouldn't be a performance issue. > > > + struct node_hstate *nhs = &node_hstates[nid]; > > + int i; > > + for (i = 0; i < HUGE_MAX_HSTATE; i++) > > + if (nhs->hstate_kobjs[i] == kobj) { > > + if (nidp) > > + *nidp = nid; > > + return &hstates[i]; > > + } > > + } > > + > > + BUG(); > > + return NULL; > > +} > > + <snip> > > + > > +/* > > + * hugetlb init time: register hstate attributes for all registered > > + * node sysdevs. All on-line nodes should have registered their > > + * associated sysdev by the time the hugetlb module initializes. > > + */ > > +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? already answered this. > > > + > > + /* > > + * Let the node sysdev driver know we're here so it can > > + * [un]register hstate attributes on node hotplug. > > + */ > > + register_hugetlbfs_with_node(hugetlb_register_node, > > + hugetlb_unregister_node); > > +} > > +#else /* !CONFIG_NUMA */ > > Index: linux-2.6.31-mmotm-090925-1435/include/linux/node.h > > =================================================================== > > --- linux-2.6.31-mmotm-090925-1435.orig/include/linux/node.h 2009-10-07 12:31:51.000000000 -0400 > > +++ linux-2.6.31-mmotm-090925-1435/include/linux/node.h 2009-10-07 12:32:01.000000000 -0400 > > @@ -28,6 +28,7 @@ struct node { > > > > struct memory_block; > > extern struct node node_devices[]; > > +typedef void (*node_registration_func_t)(struct node *); > > > > extern int register_node(struct node *, int, struct node *); > > extern void unregister_node(struct node *node); > > I previously suggested against the typedef unless this functionality (node > hotplug notifiers) becomes more generic outside of the hugetlb use case. I'd like to keep it. I've read the CodingStyle and I know it argues against typedefs, but the strongest prohibition is against [pointers to] structs whose members could be reasonable accessed. I don't think I violate that. And, this does allow the registration function definitions that take the func pointer as an argument to show up in cscope. I find that useful. Wish they all did [func defs with func args show up in cscope, that is]. But, if you and others feel strongly about this, I suppose we can rip it out. -- To unsubscribe from this list: send the line "unsubscribe linux-numa" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html