Message-ID: <20100629085532.GE22344@elte.hu>
Date:	Tue, 29 Jun 2010 10:55:32 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Lin Ming <ming.m.lin@...el.com>
Cc:	Johannes Berg <johannes@...solutions.net>,
	Peter Zijlstra <peterz@...radead.org>,
	Greg KH <greg@...ah.com>,
	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Paul Mundt <lethal@...ux-sh.org>,
	"eranian@...il.com" <eranian@...il.com>,
	"Gary.Mohr@...l.com" <Gary.Mohr@...l.com>,
	"arjan@...ux.intel.com" <arjan@...ux.intel.com>,
	"Zhang, Yanmin" <yanmin_zhang@...ux.intel.com>,
	Paul Mackerras <paulus@...ba.org>,
	"David S. Miller" <davem@...emloft.net>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Will Deacon <will.deacon@....com>,
	Maynard Johnson <mpjohn@...ibm.com>,
	Carl Love <carll@...ibm.com>,
	Kay Sievers <kay.sievers@...y.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [rfc] Describe events in a structured way via sysfs


* Lin Ming <ming.m.lin@...el.com> wrote:

> On Fri, 2010-06-25 at 01:33 +0800, Ingo Molnar wrote:
> > * Johannes Berg <johannes@...solutions.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)

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

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

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?

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

Powered by blists - more mailing lists