lkml.org 
[lkml]   [2011]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/6] perf: Add interface to add general events to sysfs
From
Date
On Mon, 2011-07-18 at 21:34 +0800, Peter Zijlstra wrote:
> On Fri, 2011-07-15 at 14:34 +0000, Lin Ming wrote:
> > PMU can implement ::add_events() to add its general events to sysfs.
> >
> > For example,
> >
> > /sys/bus/event_source/devices/uncore/events
> > └── cycle
> >
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> > include/linux/perf_event.h | 9 +++++++
> > kernel/events/core.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 61 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 3f2711c..14337a3 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -676,6 +676,14 @@ struct pmu {
> > * for each successful ->add() during the transaction.
> > */
> > void (*cancel_txn) (struct pmu *pmu); /* optional */
> > +
> > + void (*add_events) (void);
> > +};
>
> I'd rather not have a function pointer here, and all the kobj thingies
> can be hooking into the existing struct device, right?

Right.

>
> > +struct pmu_event {
> > + char name[64];
> > + u64 config;
> > + struct kobj_attribute attr;
> > };
>
> Can't this live in kerne/event/internal.h?

Yes, it can, with the new interface
perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)

>
> > /**
> > @@ -941,6 +949,7 @@ struct perf_output_handle {
> >
> > extern int perf_pmu_register(struct pmu *pmu, char *name, int type);
> > extern void perf_pmu_unregister(struct pmu *pmu);
> > +extern void perf_pmu_add_events(struct pmu *pmu, struct pmu_event events[], int count);
> >
> > extern int perf_num_counters(void);
> > extern const char *perf_pmu_name(void);
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 0567e32..9f4f463 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5571,6 +5571,8 @@ static int pmu_dev_alloc(struct pmu *pmu)
> > if (ret)
> > goto free_dev;
> >
> > + if (pmu->add_events)
> > + pmu->add_events();
> > out:
> > return ret;
> >
> > @@ -7035,4 +7037,54 @@ struct cgroup_subsys perf_subsys = {
> > .exit = perf_cgroup_exit,
> > .attach_task = perf_cgroup_attach_task,
> > };
> > +
> > +static ssize_t pmu_event_show(struct kobject *kobj,
> > + struct kobj_attribute *attr, char *buf)
> > +{
> > + struct pmu_event *event = container_of(attr, struct pmu_event, attr);
> > + int size;
> > +
> > + size = sprintf(buf, "%lld\n", event->config);
> > +
> > + return size;
> > +}
> > +
> > +void perf_pmu_add_events(struct pmu *pmu, struct pmu_event events[], int count)
> > +{
> > + struct attribute_group *attr_group = NULL;
> > + struct attribute **all_attrs = NULL;
> > + struct kobj_attribute *event_attr;
> > + struct pmu_event *event;
> > + int i;
> > +
> > + attr_group = kzalloc(sizeof(*attr_group), GFP_KERNEL);
> > + if (!attr_group)
> > + return;
> > + attr_group->name = "events";
> > +
> > + all_attrs = kzalloc(sizeof(*all_attrs) * (count + 1), GFP_KERNEL);
> > + if (!all_attrs)
> > + goto fail;
> > +
> > + for (i = 0; i < count; i++) {
> > + event = &events[i];
> > + event_attr = &event->attr;
> > +
> > + sysfs_attr_init(&event_attr->attr);
> > + event_attr->attr.name = event->name;
> > + event_attr->attr.mode = 0444;
> > + event_attr->show = pmu_event_show;
> > +
> > + all_attrs[i] = &event_attr->attr;
> > + }
> > +
> > + attr_group->attrs = all_attrs;
> > + if (!sysfs_create_group(&pmu->dev->kobj, attr_group))
> > + return;
> > +
> > +fail:
> > + kfree(attr_group);
> > + kfree(all_attrs);
> > +}
> > +
> > #endif /* CONFIG_CGROUP_PERF */
>
> Uhm, why does this live under CONFIG_CGROUP_PERF?

Sorry, my mistake. I meant to put it at the end of the file.

>
> Also, I would prefer an interface like:
>
> int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)
> {
> }
>
> which would create the events directory if not already present and
> allocate and add a pmu_sysfs_event thingy.

That's better.

Thanks.


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

\
 
 \ /
  Last update: 2011-07-18 16:03    [from the cache]
©2003-2011 Jasper Spaans