Re: [PATCH v2 1/6] perf: Add interface to add general events to sysfs

[Lin Ming <ming.m.lin@intel.com>]

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.