Re: [PATCH 4/5] mm: add numa node symlink for cpu devices in sysfs

[Alex Chiang]

* David Rientjes <rientjes@xxxxxxxxxx>:
> On Mon, 19 Oct 2009, Alex Chiang wrote:
> 
> > You can discover which CPUs belong to a NUMA node by examining
> > /sys/devices/system/node/$node/
> > 
> 
> You mean /sys/devices/system/node/node# ?

Hm, in PCI land, I've been using $foo to indicate a variable in
documentation I've written, but I can certainly use foo# if
that's the preferred style.

> > However, it's not convenient to go in the other direction, when looking at
> > /sys/devices/system/cpu/$cpu/
> > 
> 
> .../cpu/cpu# ?
> 
> > Yes, you can muck about in sysfs, but adding these symlinks makes
> > life a lot more convenient.
> > 
> > Signed-off-by: Alex Chiang <achiang@xxxxxx>
> > ---
> > 
> >  drivers/base/node.c |    9 ++++++++-
> >  1 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/base/node.c b/drivers/base/node.c
> > index ffda067..47a4997 100644
> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -227,6 +227,7 @@ struct node node_devices[MAX_NUMNODES];
> >   */
> >  int register_cpu_under_node(unsigned int cpu, unsigned int nid)
> >  {
> > +	int ret;
> >  	struct sys_device *obj;
> >  
> >  	if (!node_online(nid))
> > @@ -236,9 +237,13 @@ int register_cpu_under_node(unsigned int cpu, unsigned int nid)
> >  	if (!obj)
> >  		return 0;
> >  
> > -	return sysfs_create_link(&node_devices[nid].sysdev.kobj,
> > +	ret = sysfs_create_link(&node_devices[nid].sysdev.kobj,
> >  				&obj->kobj,
> >  				kobject_name(&obj->kobj));
> > +
> > +	return sysfs_create_link(&obj->kobj,
> > +				 &node_devices[nid].sysdev.kobj,
> > +				 kobject_name(&node_devices[nid].sysdev.kobj));
> >  }
> >  
> >  int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
> 
> That can't be right, you're ignoring the return value of the first 
> sysfs_create_link().

This was a simple oversight. my intent was to return early if the
first call to sysfs_create_link() failed.

> The return values of register_cpu_under_node() and 
> unregister_cpu_under_node() are always ignored, so it would probably be 
> best to convert these to be void functions.  That doesn't mean you can 
> simply ignore the result of the first sysfs_create_link(), though: the 
> second should probably be suppressed if the first returns an error.
> 

I didn't want to change too much in the patch. Changing the
function signature seems a bit overeager, but if you have strong
feelings, I can do so.

Thanks for the review.

/ac

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/