On Fri, 9 Oct 2009, Lee Schermerhorn wrote:
> > > +/*
> > > + * 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.
>
Hmm, does this really work for memory hot-remove? If all memory is
removed from a nid, does node_hstates[nid]->hstate_objs[] get updated
appropriately? I assume we'd never pass that particular kobj to
kobj_to_node_hstate() anymore, but I'm wondering if the pointer would
remain in the hstate_kobjs[] table.
> > > 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.
>
Ok, I agree that it would be convenient if this could evolve into a
generic node hotplug notifier taht can be used all over the kernel. I
don't see any reason why that can't happen based on the work you've done
in this particular patch, so I have no strong objection to it (although
maybe it would be better named `node_notifier_func_t' since it unregisters
nodes too?).
--
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
