Re: [RFC][PATCH v2 08/11] sysfs: introduce new interface sysfs_get_kobject

[Greg KH <greg@kroah.com>]

On Wed, May 19, 2010 at 10:55:17AM +0800, Lin Ming wrote:
> On Wed, 2010-05-19 at 04:08 +0800, Greg KH wrote:
> > On Wed, May 19, 2010 at 01:48:43AM +0000, Lin Ming wrote:
> > > Need this interface in the later sysfs pmu lookup.
> > > 
> > > struct kobject *sysfs_get_kobject(struct file *file);
> > > Return the relative kobject of the sysfs file.
> > 
> > Ick, no.  Why would you ever have the file, yet not have the kobject
> > already?  Something is really wrong if this is needed.  Or strange.  Or
> > maybe both :)
> 
> Let me show you the scenario.
> 
> /sys/devices/system/cpu/event_source/
> `-- id
> 
> /sys/devices/system/cpu/events/
> |-- L1-dcache-load-misses
> |   |-- event_source -> ../../event_source
> 
> $perf top -e L1-dcache-load-misses
> 
> Lookup the pmu used to handle L1-dcache-load-misses as below,
> 
> 1. pmu_sys_fd =
> open("/sys/devices/system/cpu/events/L1-dcache-load-misses/event_source/id", ...)

You do that within the kernel?  or from userspace?

Either way, your id show callback will get called when you read or write
to that file, right?  Then you have your kobject.

> 2. pmu_sys_file = fget_light(pmu_sys_fd, ....)
> 
> 3. pmu_kobject = sysfs_get_kobject(pmu_sys_file)
> 
> 4. pmu_kobject is embedded in struct pmu, pmu = container_of(kobj,
> struct pmu, kobj);

Oh no.

You are just using the kobject sysfs tree to store your kobjects so you
can look them up again some time in the future from within the kernel?
Seriously?

What's wrong with a simple list of kobjects?  Or what the rest of the
kernel does (busses and devices and iterating over the devices for a
bus)?

Don't act like userspace here and try to use the sysfs filesystem layout
as a lookup into the kobject you are trying to find.  That's a
horrible abuse of sysfs.  One of the worse I have ever seen.  And I've
seen a lot of sysfs abuse over the years...

ick.

greg k-h