Describe events in a structured way via sysfs
in [Kernel]
|
From: Ingo Molnar on 24 Jun 2010 05:40 * Johannes Berg <johannes(a)sipsolutions.net> wrote: > On Tue, 2010-06-22 at 15:22 +0800, Lin Ming wrote: > > > > > net/wlan0/events/ > > > > net/waln1/events/ > > > > .... > > > > net/walnN/events/ > > > > > > That's not appropriate either though since you may have multiple network > > > interfaces on the same hardware :) > > > > Doesn't net/wlan0...wlanN mean multiple network interfaces on the same > > hardware? > > Yes, but the trace points aren't per network interface but rather per > hardware piece. Yeah - we generally want events to live at their 'natural' source in sysfs. So if it's a per device hardware event, it should live with the hardware piece. If it's a higher level chipset event, it should live where the chipset driver is in sysfs. If it's a subsystem level event then it should live there. I think what you mentioned in your other posting makes the most sense: give flexibility to tracepoint authors to place the event in the most sensible sysfs place. It is them who define the tracepoints and the events so any second guessing by a generic layer will probably get in the way. The generic tool layer will be content with having the event_source class in sysfs, to see 'all' event sources ttheir topological structure. 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. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Berg on 24 Jun 2010 12:20 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. 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. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 24 Jun 2010 13:40 * Johannes Berg <johannes(a)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. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Lin Ming on 29 Jun 2010 02:20 On Fri, 2010-06-25 at 01:33 +0800, Ingo Molnar wrote: > * Johannes Berg <johannes(a)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; } static void diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 716f99b..2a6d834 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1019,6 +1019,8 @@ extern int perf_swevent_get_recursion_context(void); extern void perf_swevent_put_recursion_context(int rctx); extern void perf_event_enable(struct perf_event *event); extern void perf_event_disable(struct perf_event *event); + +extern void perf_sys_register_tp(struct kobject *kobj, char *tp_system); #else static inline void perf_event_task_sched_in(struct task_struct *task) { } diff --git a/kernel/perf_event.c b/kernel/perf_event.c index 403d180..1b85dad 100644 --- a/kernel/perf_event.c +++ b/kernel/perf_event.c @@ -5877,3 +5877,32 @@ static int __init perf_event_sysfs_init(void) &perfclass_attr_group); } device_initcall(perf_event_sysfs_init); + +#define for_each_event(event, start, end) \ + for (event = start; \ + (unsigned long)event < (unsigned long)end; \ + event++) + +extern struct ftrace_event_call __start_ftrace_events[]; +extern struct ftrace_event_call __stop_ftrace_events[]; + +void perf_sys_register_tp(struct kobject *kobj, char *tp_system) +{ + struct ftrace_event_call *call; + struct kobject *events_kobj; + + events_kobj = kobject_create_and_add("events", kobj); + if (!events_kobj) + return; + + for_each_event(call, __start_ftrace_events, __stop_ftrace_events) { + if (call->class->system && !strcmp(call->class->system, tp_system)) { + + /* create events/<tracepoint> */ + kobject_create_and_add(call->name, events_kobj); + + /* create events/<tracepoint>/enable, filter, format, id */ + /* TBD ... */ + } + } +} -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 29 Jun 2010 05:00
* Lin Ming <ming.m.lin(a)intel.com> wrote: > On Fri, 2010-06-25 at 01:33 +0800, Ingo Molnar wrote: > > * Johannes Berg <johannes(a)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) 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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |