Re: [rfc] Describe events in a structured way via sysfs

[Lin Ming <ming.m.lin@intel.com>]

On Tue, 2010-06-29 at 16:55 +0800, Ingo Molnar wrote:
> * Lin Ming <ming.m.lin@intel.com> wrote:
> 
> > On Fri, 2010-06-25 at 01:33 +0800, Ingo Molnar wrote:
> > > * Johannes Berg <johannes@sipsolutions.net> wrote:
> > > 
> > > > On Thu, 2010-06-24 at 11:36 +0200, Ingo Molnar wrote:
> > > > 
> > > > > That's probably best achieved via a TRACE_EVENT() variant, by passing in the 
> > > > > sysfs location.
> > > > > 
> > > > > It might even make sense to make this a part of TRACE_EVENT() itself and make 
> > > > > 'NULL' the current default, non-sysfs-enumerated behavior. That way we can 
> > > > > gradually (and non-intrusively) find all the right sysfs places for events.
> > > > 
> > > > No, this doesn't work. A lot of events are multi-instance. Say you have an 
> > > > event for each USB device. This event would have to show up in many places 
> > > > in sysfs, and each trace_foo() invocation needs to get the struct device 
> > > > pointer, not just the TRACE_EVENT() definition. Additionally, to 
> > > > create/destroy the sysfs pieces we need something like init_trace_foo(dev) 
> > > > and destroy_trace_foo(dev) be called when the sysfs points for the device 
> > > > should be created/destroyed.
> > > 
> > > Yes - but even this could be expressed via TRACE_EVENT(): by giving it a 
> > > device-specific function pointer and then instantiating individual events from 
> > > a single, central place in sysfs.
> > > 
> > > That is the place where we already know where it ends up in sysfs, and where 
> > > the event-specific function can match up whether that particular node belongs 
> > > to it and whether an additional event directory should be created for that 
> > > particular sysfs node.
> > > 
> > > > The TRACE_EVENT() just defines the template, but such multi-instance events 
> > > > really should be standardised in terms of their struct device (or maybe 
> > > > kobject).
> > > > 
> > > > I think that needs some TRACE_DEVICE_EVENT macro that creates the required 
> > > > inlines etc, and including the init/destroy that are called when the event 
> > > > should show up in sysfs.
> > > > 
> > > > There's no way you can have the event show up in sysfs at the right spot 
> > > > with _just_ a TRACE_EVENT macro, since at define time in the header file you 
> > > > don't even have a valid struct device pointer.
> > > 
> > > That would be another possible way to do it - to explicitly create the events 
> > > directory. It looks a bit simpler as we wouldnt have to touch TRACE_EVENT() 
> > > and because it directly expresses the 'this node has an events directory' 
> > > property at the place where we create the device node.
> > 
> > Let me take i915 tracepoints as an example.
> > Do you mean something like below?
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 423dc90..9e7e4a0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -28,6 +28,7 @@
> >   */
> >  
> >  #include <linux/device.h>
> > +#include <linux/perf_event.h>
> >  #include "drmP.h"
> >  #include "drm.h"
> >  #include "i915_drm.h"
> > @@ -413,7 +414,17 @@ int i965_reset(struct drm_device *dev, u8 flags)
> >  static int __devinit
> >  i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  {
> > -	return drm_get_dev(pdev, ent, &driver);
> > +	struct kobject *kobj;
> > +	int ret;
> > +
> > +	ret = drm_get_dev(pdev, ent, &driver);
> > +
> > +	if (!ret) {
> > +		kobj = &pdev->dev.kobj;
> > +		perf_sys_register_tp(kobj, "i915");
> > +	}
> > +
> > +	return ret;
> 
> Yeah, something like that - assuming that this means that we'll add the events 
> directory to the device directory, to all the 
> /sys/bus/pci/drivers/i915/*/events/ driver directories right? (i havent 
> checked the DRM code)

I haven't run the code, but I think yes.

> 
> Small detail, it could be written a bit more compactly, like:

Thanks for the tip.

> 
> > +	int ret;
> > +
> > +	ret = drm_get_dev(pdev, ent, &driver);
> > +	if (!ret)
> > +		perf_sys_register_tp(&pdev->dev.kobj, "i915");
> > +
> > +	return ret;
> 
> Also, we can (optionally) consider 'generic', subsystem level events to also 
> show up under:
> 
>    /sys/bus/pci/drivers/i915/events/
> 
> This would give a model to non-device-specific events to be listed one level 
> higher in the sysfs hierarchy.
> 
> This too would be done in the driver, not by generic code. It's generally the 
> driver which knows how the events should be categorized.

This is a bit difficult. I'd like not to touch TRACE_EVENT().
How does the driver know if an event is 'generic' if TRACE_EVENT is not
touched?

> 
> I'd imagine something similar for wireless drivers as well - most currently 
> defined events would show up on a per device basis there.
> 
> Can you see practical problems with this scheme?

Not now. I may find some problems when write more detail code.

Lin Ming

> 
> 	Ingo