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

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

[Greg KH]

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 :)

thanks,

greg k-h

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

[Lin Ming]

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.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 fs/sysfs/file.c       |   15 +++++++++++++++
 include/linux/sysfs.h |    2 ++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e222b25..0c5d6fe 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -748,6 +748,21 @@ int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
 }
 EXPORT_SYMBOL_GPL(sysfs_schedule_callback);
 
+struct kobject *sysfs_get_kobject(struct file *file)
+{
+	struct sysfs_dirent *attr_sd;
+	struct kobject *kobj;
+
+	if (!file)
+		return NULL;
+	if (file->f_op != &sysfs_file_operations)
+		return NULL;
+
+	attr_sd = file->f_path.dentry->d_fsdata;
+	kobj = attr_sd->s_parent->s_dir.kobj;
+
+	return kobj;
+}
 
 EXPORT_SYMBOL_GPL(sysfs_create_file);
 EXPORT_SYMBOL_GPL(sysfs_remove_file);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f0496b3..5954234 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -121,6 +121,8 @@ struct sysfs_dirent;
 
 #ifdef CONFIG_SYSFS
 
+struct kobject *sysfs_get_kobject(struct file *file);
+
 int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
 			    void *data, struct module *owner);
 

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

[Lin Ming]

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", ...)

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);

> 
> thanks,
> 
> greg k-h

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

[Greg KH]

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

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

[Lin Ming]

On Wed, 2010-05-19 at 11:35 +0800, Greg KH wrote:
> 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?

>From userspace.
See "[RFC][PATCH v2 11/11] perf top: demo of how to use the sysfs
interfac" for a simple example.

> 
> 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?

Yes.

> 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)?

A simple list of pmus is also added in this patch series

There are 2 pmu lookup methods in this patch series.
1. Search a simple list of pmus
2. Lookup pmu via sysfs 

Method 1 is used for back compatibility.

> 
> 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...

Ah, it's that bad... I need to re-think how to export pmus/events to
userspace...

Lin Ming

> 
> ick.
> 
> greg k-h