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    [W:0.032 / U:30.120 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site