Message-Id: <1278057996.3841.26.camel@minggr.sh.intel.com>
Date:	Fri, 02 Jul 2010 16:06:36 +0800
From:	Lin Ming <ming.m.lin@...el.com>
To:	Ingo Molnar <mingo@...e.hu>
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

On Tue, 2010-06-29 at 18:26 +0800, Ingo Molnar wrote:
> * Lin Ming <ming.m.lin@...el.com> wrote:
> 
> > > 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(). [...]
> 
> We can certainly start with the simpler variant - it's also the more common 
> case.
> 
> > [...] How does the driver know if an event is 'generic' if TRACE_EVENT is 
> > not touched?
> 
> Well, it's per driver code which creates the 'events' directory anyway, so 
> that code decides where to link things. It can link it to the per driver kobj 
> - or to the per subsys kobj.
> 
> > > 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.
> 
> Ok. Feel free to post RFC patches (even if they are not fully complete yet), 
> so that we can see how things are progressing.
> 
> I suspect the best approach would be to try to figure out the right sysfs 
> placement for one or two existing driver tracepoints, so that we can see it 
> all in practice. (Obviously any changes to drivers will have to go via the 
> relevant driver maintainer tree(s).)

Well, take i915 tracepoints as an example, the sys structures as below

/sys/class/drm/card0/events/
|-- i915_gem_object_bind
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_change_domain
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_clflush
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_create
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_destroy
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_get_fence
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_object_unbind
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_complete
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_flush
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_retire
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_submit
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_wait_begin
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_gem_request_wait_end
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
|-- i915_ring_wait_begin
|   |-- enable
|   |-- filter
|   |-- format
|   `-- id
`-- i915_ring_wait_end
    |-- enable
    |-- filter
    |-- format
    `-- id

And below is the very draft patch to export i915 tracepoints in sysfs.
Is it the right direction?

---
 drivers/gpu/drm/i915/i915_drv.c |   15 +++-
 include/linux/perf_event.h      |    2 +
 kernel/perf_event.c             |  168 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 423dc90..eb7fa9e 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,19 @@ 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;
+	struct drm_device *drm_dev;
+	int ret;
+
+	ret = drm_get_dev(pdev, ent, &driver);
+
+	if (!ret) {
+		drm_dev = pci_get_drvdata(pdev);
+		kobj = &drm_dev->primary->kdev.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..068ee48 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -5877,3 +5877,171 @@ 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[];
+extern void print_event_filter(struct ftrace_event_call *call,
+                               struct trace_seq *s);
+
+struct tp_kobject {
+	struct kobject *kobj;
+	struct ftrace_event_call *call;
+	struct tp_kobject *next;
+};
+
+static struct tp_kobject *tp_kobject_list;
+
+static struct ftrace_event_call *perf_sys_find_tp_call(struct kobject *kobj)
+{
+	struct tp_kobject *tp_kobj;
+
+	tp_kobj = tp_kobject_list;
+
+	while (tp_kobj) {
+		if (kobj == tp_kobj->kobj)
+			return tp_kobj->call;
+
+		tp_kobj = tp_kobj->next;
+	}
+
+	return NULL;
+}
+
+#define TP_ATTR_RO(_name) \
+	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
+
+#define TP_ATTR(_name) \
+	static struct kobj_attribute _name##_attr = \
+		__ATTR(_name, 0644, _name##_show, _name##_store)
+
+static ssize_t enable_show(struct kobject *kobj,
+	struct kobj_attribute *attr, char *buf)
+{
+	struct ftrace_event_call *call;
+
+	call = perf_sys_find_tp_call(kobj);
+	return sprintf(buf, "%d\n", call->flags & TRACE_EVENT_FL_ENABLED);
+}
+
+static ssize_t enable_store(struct kobject *kobj,
+	struct kobj_attribute *attr, const char *buf, size_t len)
+{
+	/* Not implemented yet */
+
+	return 0;
+}
+TP_ATTR(enable);
+
+static ssize_t filter_show(struct kobject *kobj,
+	struct kobj_attribute *attr, char *buf)
+{
+	struct ftrace_event_call *call;
+	struct trace_seq *s;
+
+	call = perf_sys_find_tp_call(kobj);
+
+	s = kmalloc(sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	trace_seq_init(s);
+
+	print_event_filter(call, s);
+
+	memcpy(buf, s->buffer, s->len);
+
+	kfree(s);
+
+	return s->len;
+}
+
+static ssize_t filter_store(struct kobject *kobj,
+	struct kobj_attribute *attr, const char *buf, size_t len)
+{
+	/* Not implemented yet */
+
+	return 0;
+}
+TP_ATTR(filter);
+
+static ssize_t format_show(struct kobject *kobj,
+	struct kobj_attribute *attr, char *buf)
+{
+	/* Not implemented yet */
+
+	return 0;
+}
+TP_ATTR_RO(format);
+
+static ssize_t id_show(struct kobject *kobj,
+	struct kobj_attribute *attr, char *buf)
+{
+	struct ftrace_event_call *call;
+
+	call = perf_sys_find_tp_call(kobj);
+
+	return sprintf(buf, "%d\n", call->event.type);
+}
+TP_ATTR_RO(id);
+
+static struct attribute *tp_attrs[] = {
+	&enable_attr.attr,
+	&filter_attr.attr,
+	&format_attr.attr,
+	&id_attr.attr,
+	NULL,
+};
+
+static struct attribute_group tp_attr_group = {
+	.attrs = tp_attrs,
+};
+
+static int perf_sys_add_tp(struct kobject *parent, struct ftrace_event_call *call)
+{
+	struct tp_kobject *tp_kobj;
+	struct kobject *event_kobj;
+	int err;
+
+	event_kobj = kobject_create_and_add(call->name, parent);
+	if (!event_kobj)
+		return -ENOMEM;
+	err = sysfs_create_group(event_kobj, &tp_attr_group);
+	if (err) {
+		kobject_put(event_kobj);
+		return -ENOMEM;
+	}
+
+	tp_kobj = kmalloc(sizeof(*tp_kobj), GFP_KERNEL);
+	if (!tp_kobj) {
+		kobject_put(event_kobj);	
+		return -ENOMEM;
+	}	
+
+	tp_kobj->kobj = event_kobj;
+	tp_kobj->call = call;
+	tp_kobj->next = tp_kobject_list;
+	tp_kobject_list = tp_kobj;	
+
+	return 0;
+}
+
+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)) {
+			perf_sys_add_tp(events_kobj, call);
+		}
+	}
+}


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