[PATCH v2 0/6] perf: Intel uncore pmu counting support

[PATCH v2 0/6] perf: Intel uncore pmu counting support

[Lin Ming]

Hi, all

This is the v2 patches to support uncore pmu counting.
The most important change is to use hrtimer to pull uncore counters
to handle overflow, because uncore pmu interrupt does not work.

v2 -> v1:
- Use hrtimer to pull counters
- Add interface to add general events to sysfs
- Move MSRs to msr-index.h
- Add uncore fixed counter
- Allow system-wide 'perf stat' without 'command'

Comments are appreciated.

Thanks,
Lin Ming

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

[Lin Ming]

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);
+};
+
+struct pmu_event {
+	char name[64];
+	u64 config;
+	struct kobj_attribute attr;
 };
 
 /**
@@ -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 */
-- 
1.7.5.1

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

[Peter Zijlstra]

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?

> +struct pmu_event {
> +	char name[64];
> +	u64 config;
> +	struct kobj_attribute attr;
>  };

Can't this live in kerne/event/internal.h?

>  /**
> @@ -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?

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.

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

[Lin Ming]

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.

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

[Lin Ming]

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?

I forgot to mention one important reason why I added a function pointer.

The events can only be added to sysfs after PMU sysfs is initialized in
perf_event_sysfs_init ->
    pmu_dev_alloc

> @@ -5571,6 +5571,8 @@ static int pmu_dev_alloc(struct pmu *pmu)
>        if (ret)
>                goto free_dev;
>
> +       if (pmu->add_events)
> +               pmu->add_events();

So we need a pmu callback which is called in pmu_dev_alloc to add events
to sysfs.

You suggested a new interface,
int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)

But where should it be called?
I guess you mean to call it in pmu init function, for example,
uncore_pmu_init.

But pmu init function maybe called before perf_event_sysfs_init, which
means the pmu sysfs has not been initialized yet.

Lin Ming

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

[Lin Ming]

On Tue, 2011-07-19 at 15:52 +0800, Lin Ming wrote:
> 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?
> 
> I forgot to mention one important reason why I added a function pointer.
> 
> The events can only be added to sysfs after PMU sysfs is initialized in
> perf_event_sysfs_init ->
>     pmu_dev_alloc
> 
> > @@ -5571,6 +5571,8 @@ static int pmu_dev_alloc(struct pmu *pmu)
> >        if (ret)
> >                goto free_dev;
> >
> > +       if (pmu->add_events)
> > +               pmu->add_events();
> 
> So we need a pmu callback which is called in pmu_dev_alloc to add events
> to sysfs.
> 
> You suggested a new interface,
> int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)
> 
> But where should it be called?
> I guess you mean to call it in pmu init function, for example,
> uncore_pmu_init.
> 
> But pmu init function maybe called before perf_event_sysfs_init, which
> means the pmu sysfs has not been initialized yet.

Hi, Peter

Will you have a chance to look at this?

Thanks.

> 
> Lin Ming

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

[Peter Zijlstra]

On Tue, 2011-07-19 at 15:52 +0800, Lin Ming wrote:

> > I'd rather not have a function pointer here, and all the kobj thingies
> > can be hooking into the existing struct device, right?
> 
> I forgot to mention one important reason why I added a function pointer.
> 
> The events can only be added to sysfs after PMU sysfs is initialized in
> perf_event_sysfs_init ->
>     pmu_dev_alloc

Right.

> > @@ -5571,6 +5571,8 @@ static int pmu_dev_alloc(struct pmu *pmu)
> >        if (ret)
> >                goto free_dev;
> >
> > +       if (pmu->add_events)
> > +               pmu->add_events();
> 
> So we need a pmu callback which is called in pmu_dev_alloc to add events
> to sysfs.
> 
> You suggested a new interface,
> int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)
> 
> But where should it be called?
> I guess you mean to call it in pmu init function, for example,
> uncore_pmu_init.
> 
> But pmu init function maybe called before perf_event_sysfs_init, which
> means the pmu sysfs has not been initialized yet.

Right, so there is no reason to call perf_pmu_register() really early,
except for the normal pmu and the software pmu (since they're used by
the watchdog muck).

The uncore for example can use late_initcall() just fine and use both
perf_pmu_register() and the proposed perf_pmu_add_event() from the same 
init call.

Only for the primary pmu and software thingies do we need to add an
extra init call.

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

[Lin Ming]

On Mon, 2011-07-25 at 15:57 +0800, Peter Zijlstra wrote:
> On Tue, 2011-07-19 at 15:52 +0800, Lin Ming wrote:
> 
> > > I'd rather not have a function pointer here, and all the kobj thingies
> > > can be hooking into the existing struct device, right?
> > 
> > I forgot to mention one important reason why I added a function pointer.
> > 
> > The events can only be added to sysfs after PMU sysfs is initialized in
> > perf_event_sysfs_init ->
> >     pmu_dev_alloc
> 
> Right.
> 
> > > @@ -5571,6 +5571,8 @@ static int pmu_dev_alloc(struct pmu *pmu)
> > >        if (ret)
> > >                goto free_dev;
> > >
> > > +       if (pmu->add_events)
> > > +               pmu->add_events();
> > 
> > So we need a pmu callback which is called in pmu_dev_alloc to add events
> > to sysfs.
> > 
> > You suggested a new interface,
> > int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)
> > 
> > But where should it be called?
> > I guess you mean to call it in pmu init function, for example,
> > uncore_pmu_init.
> > 
> > But pmu init function maybe called before perf_event_sysfs_init, which
> > means the pmu sysfs has not been initialized yet.
> 
> Right, so there is no reason to call perf_pmu_register() really early,
> except for the normal pmu and the software pmu (since they're used by
> the watchdog muck).
> 
> The uncore for example can use late_initcall() just fine and use both
> perf_pmu_register() and the proposed perf_pmu_add_event() from the same 
> init call.
> 
> Only for the primary pmu and software thingies do we need to add an
> extra init call.

This seems a solution.

I'll use late_initcall for uncore.
Then later we can add an extra late_initcall for normal pmu and software
pmu.

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

[Lin Ming]

On Mon, 2011-07-18 at 21:34 +0800, Peter Zijlstra wrote:
> 
> 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.

It's strange that I didn't find a way to check if a directory is present
in sysfs. Any hint?

So I proceeded with below code.

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 14337a3..6db73d0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -611,6 +611,7 @@ struct pmu {
 	struct list_head		entry;
 
 	struct device			*dev;
+	struct kobject			*events;
 	char				*name;
 	int				type;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3870c106..080d684 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7081,3 +7081,35 @@ fail:
 	kfree(attr_group);
 	kfree(all_attrs);
 }
+
+#define NAME_LEN 64
+
+int perf_pmu_add_event(struct pmu *pmu, const char *name, u64 config)
+{
+	struct kobject *events = pmu->events;
+	struct pmu_event *event;
+	struct kobj_attribute *event_attr;
+
+	if (!pmu_bus_running)
+		return -EINVAL;
+
+	if (!events) {
+		events = kobject_create_and_add("events", &pmu->dev->kobj);
+		if (!unlikely(events))
+			return -ENOMEM;
+	}
+
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
+		return -ENOMEM;
+
+	strncpy(event->name, name, NAME_LEN);
+	event->config = config;
+	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;
+
+	return sysfs_create_file(events, &event_attr->attr);
+}

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

[Peter Zijlstra]

On Mon, 2011-07-25 at 16:11 +0800, Lin Ming wrote:
> It's strange that I didn't find a way to check if a directory is present
> in sysfs. Any hint?

sysfs is voodoo for me, best I can come up with is, ask the sysfs
folks ;-)

Kay, Greg?

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

[Greg KH]

On Mon, Jul 25, 2011 at 10:32:03AM +0200, Peter Zijlstra wrote:
> On Mon, 2011-07-25 at 16:11 +0800, Lin Ming wrote:
> > It's strange that I didn't find a way to check if a directory is present
> > in sysfs. Any hint?
> 
> sysfs is voodoo for me, best I can come up with is, ask the sysfs
> folks ;-)
> 
> Kay, Greg?

What are you trying to do here specifically?

sysfs is not ment to be walked from within the kernel, only from
userspace.  Perhaps that is your problem here?

greg k-h

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

[Lin Ming]

On Mon, 2011-07-25 at 23:20 +0800, Greg KH wrote:
> On Mon, Jul 25, 2011 at 10:32:03AM +0200, Peter Zijlstra wrote:
> > On Mon, 2011-07-25 at 16:11 +0800, Lin Ming wrote:
> > > It's strange that I didn't find a way to check if a directory is present
> > > in sysfs. Any hint?
> > 
> > sysfs is voodoo for me, best I can come up with is, ask the sysfs
> > folks ;-)
> > 
> > Kay, Greg?
> 
> What are you trying to do here specifically?

Now there are perf event source devices in sysfs.

$ tree /sys/bus/event_source/devices/
/sys/bus/event_source/devices/
|-- breakpoint
|-- cpu
|-- software
`-- tracepoint

I want to create a "events" directory to hold the most useful events
under the event_source.

$ tree /sys/bus/event_source/devices/cpu
| -- events

Then add one event each time to the "events" dir.

Add "event1"
$ tree /sys/bus/event_source/devices/cpu
| -- events
     | -- event1

Add "event2"
$ tree /sys/bus/event_source/devices/cpu
| -- events
     | -- event1
     | -- event2

So at the first time to add a event I need to check if the "events"
directory is present. If not, create it.

Like below pseudo code,

int perf_pmu_add_event(struct pmu *pmu, const char *name, ....)
{
	if there is no "events" dir under pmu->dev->kobj
		then create "events" dir

	add event "name" to "events"

	return ....
}

So the problem is, for example, how to check if there is a "events" dir
under /sys/bus/event_source/devices/cpu in kernel.

Thanks,
Lin Ming

> 
> sysfs is not ment to be walked from within the kernel, only from
> userspace.  Perhaps that is your problem here?
> 
> greg k-h

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

[Greg KH]

On Tue, Jul 26, 2011 at 09:06:07AM +0800, Lin Ming wrote:
> On Mon, 2011-07-25 at 23:20 +0800, Greg KH wrote:
> > On Mon, Jul 25, 2011 at 10:32:03AM +0200, Peter Zijlstra wrote:
> > > On Mon, 2011-07-25 at 16:11 +0800, Lin Ming wrote:
> > > > It's strange that I didn't find a way to check if a directory is present
> > > > in sysfs. Any hint?
> > > 
> > > sysfs is voodoo for me, best I can come up with is, ask the sysfs
> > > folks ;-)
> > > 
> > > Kay, Greg?
> > 
> > What are you trying to do here specifically?
> 
> Now there are perf event source devices in sysfs.

As of what kernel release?

> $ tree /sys/bus/event_source/devices/
> /sys/bus/event_source/devices/
> |-- breakpoint
> |-- cpu
> |-- software
> `-- tracepoint
> 
> I want to create a "events" directory to hold the most useful events
> under the event_source.

Then use the attribute group for that.

> $ tree /sys/bus/event_source/devices/cpu
> | -- events
> 
> Then add one event each time to the "events" dir.
> 
> Add "event1"
> $ tree /sys/bus/event_source/devices/cpu
> | -- events
>      | -- event1
> 
> Add "event2"
> $ tree /sys/bus/event_source/devices/cpu
> | -- events
>      | -- event1
>      | -- event2
> 
> So at the first time to add a event I need to check if the "events"
> directory is present. If not, create it.
> 
> Like below pseudo code,
> 
> int perf_pmu_add_event(struct pmu *pmu, const char *name, ....)
> {
> 	if there is no "events" dir under pmu->dev->kobj
> 		then create "events" dir
> 
> 	add event "name" to "events"
> 
> 	return ....
> }
> 
> So the problem is, for example, how to check if there is a "events" dir
> under /sys/bus/event_source/devices/cpu in kernel.

Just use the proper attribute group and you will be fine.  That will
create the directory automatically.

And I really don't think sysfs is good for this, as you don't have a
bus, and your devices aren't on that bus, and the subdirs you are
creating are not "normal" for sysfs.

Why not create your own filesystem?

greg k-h

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

[Lin Ming]

On Tue, 2011-07-26 at 12:42 +0800, Greg KH wrote:
> On Tue, Jul 26, 2011 at 09:06:07AM +0800, Lin Ming wrote:
> > On Mon, 2011-07-25 at 23:20 +0800, Greg KH wrote:
> > > On Mon, Jul 25, 2011 at 10:32:03AM +0200, Peter Zijlstra wrote:
> > > > On Mon, 2011-07-25 at 16:11 +0800, Lin Ming wrote:
> > > > > It's strange that I didn't find a way to check if a directory is present
> > > > > in sysfs. Any hint?
> > > > 
> > > > sysfs is voodoo for me, best I can come up with is, ask the sysfs
> > > > folks ;-)
> > > > 
> > > > Kay, Greg?
> > > 
> > > What are you trying to do here specifically?
> > 
> > Now there are perf event source devices in sysfs.
> 
> As of what kernel release?

Since v2.6.38-rc1, commit abe4340(perf: Sysfs enumeration)

[PATCH v2 2/6] perf, x86: Add Intel Nehalem/Westmere uncore pmu

[Lin Ming]

Add Intel Nehalem/Westmere uncore pmu support.
And also the generic data structure to support uncore pmu.

Uncore pmu interrupt does not work, so hrtimer is used to pull counters.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 arch/x86/include/asm/msr-index.h              |    8 +
 arch/x86/kernel/cpu/Makefile                  |    1 +
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |  450 +++++++++++++++++++++++++
 arch/x86/kernel/cpu/perf_event_intel_uncore.h |   53 +++
 4 files changed, 512 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/perf_event_intel_uncore.c
 create mode 100644 arch/x86/kernel/cpu/perf_event_intel_uncore.h

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 485b4f1..e66011e 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -421,6 +421,14 @@
 #define MSR_CORE_PERF_GLOBAL_CTRL	0x0000038f
 #define MSR_CORE_PERF_GLOBAL_OVF_CTRL	0x00000390
 
+/* Intel Nehalem/Westmere uncore performance counters */
+#define MSR_UNCORE_PERF_GLOBAL_CTRL	0x00000391
+#define MSR_UNCORE_FIXED_CTR_CTRL	0x00000394
+#define MSR_UNCORE_FIXED_CTR0		0x00000395
+
+#define MSR_NHM_UNCORE_PMC0		0x000003b0
+#define MSR_NHM_UNCORE_PERFEVTSEL0	0x000003c0
+
 /* Geode defined MSRs */
 #define MSR_GEODE_BUSCONT_CONF0		0x00001900
 
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 6042981..31fd49e 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_CPU_SUP_TRANSMETA_32)	+= transmeta.o
 obj-$(CONFIG_CPU_SUP_UMC_32)		+= umc.o
 
 obj-$(CONFIG_PERF_EVENTS)		+= perf_event.o
+obj-$(CONFIG_PERF_EVENTS)              += perf_event_intel_uncore.o
 
 obj-$(CONFIG_X86_MCE)			+= mcheck/
 obj-$(CONFIG_MTRR)			+= mtrr/
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
new file mode 100644
index 0000000..79a501e
--- /dev/null
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -0,0 +1,450 @@
+#include "perf_event_intel_uncore.h"
+
+static DEFINE_PER_CPU(struct cpu_uncore_events, cpu_uncore_events);
+static DEFINE_RAW_SPINLOCK(intel_uncore_lock);
+
+static bool uncore_pmu_initialized;
+static struct intel_uncore_pmu intel_uncore_pmu __read_mostly;
+
+/*
+ * Uncore pmu interrupt does not work.
+ * Use hrtimer to pull the counter every 10 seconds.
+ */
+#define UNCORE_PMU_HRTIMER_INTERVAL (10000000000ULL)
+
+/* Common functions for Nehalem/Westmere/SandyBridge */
+
+static void uncore_pmu_disable_all(void)
+{
+	wrmsrl(MSR_UNCORE_PERF_GLOBAL_CTRL, 0);
+}
+
+static void uncore_fixed_hw_config(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	hwc->config_base = MSR_UNCORE_FIXED_CTR_CTRL;
+	hwc->event_base = MSR_UNCORE_FIXED_CTR0;
+}
+
+static void uncore_fixed_disable_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	wrmsrl(hwc->config_base, 0);
+}
+
+static void uncore_pmu_enable_event(struct perf_event *event, u64 fixed_enable)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (event->hw.idx == X86_PMC_IDX_FIXED) {
+		wrmsrl(hwc->config_base, fixed_enable);
+		return;
+	}
+
+	wrmsrl(hwc->config_base,
+	       hwc->config | UNCORE_EVENTSEL_ENABLE);
+}
+
+static void uncore_pmu_disable_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (event->hw.idx == X86_PMC_IDX_FIXED) {
+		uncore_fixed_disable_event(event);
+		return;
+	}
+
+	wrmsrl(hwc->config_base, hwc->config);
+}
+
+/* Nehalem/Westmere uncore pmu */
+
+static void nhm_uncore_pmu_enable_all(void)
+{
+	u64 ctrl = (1 << UNCORE_NUM_COUNTERS) - 1;
+
+	wrmsrl(MSR_UNCORE_PERF_GLOBAL_CTRL, ctrl);
+}
+
+static void nhm_uncore_pmu_enable_event(struct perf_event *event)
+{
+	uncore_pmu_enable_event(event, NHM_UNCORE_FIXED_CTR_CTRL_EN);
+}
+
+static void nhm_uncore_pmu_hw_config(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (event->hw.idx == X86_PMC_IDX_FIXED) {
+		uncore_fixed_hw_config(event);
+		return;
+	}
+
+	hwc->config = event->attr.config & NHM_UNCORE_RAW_EVENT_MASK;
+	hwc->config_base = MSR_NHM_UNCORE_PERFEVTSEL0 + hwc->idx;
+	hwc->event_base = MSR_NHM_UNCORE_PMC0 + hwc->idx;
+}
+
+static __initconst const struct intel_uncore_pmu nhm_uncore_pmu = {
+	.name			= "Nehalem/Westmere",
+	.disable_all		= uncore_pmu_disable_all,
+	.enable_all		= nhm_uncore_pmu_enable_all,
+	.enable			= nhm_uncore_pmu_enable_event,
+	.disable		= uncore_pmu_disable_event,
+	.hw_config		= nhm_uncore_pmu_hw_config,
+	.cntval_bits		= 48,
+	.cntval_bits_fixed	= 48,
+};
+
+static u64 uncore_perf_event_update(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	int shift;
+	u64 prev_raw_count, new_raw_count;
+	s64 delta;
+
+	if (event->hw.idx == X86_PMC_IDX_FIXED)
+		shift = 64 - intel_uncore_pmu.cntval_bits_fixed;
+	else
+		shift = 64 - intel_uncore_pmu.cntval_bits;
+
+again:
+	prev_raw_count = local64_read(&hwc->prev_count);
+	rdmsrl(hwc->event_base, new_raw_count);
+
+	if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
+			    new_raw_count) != prev_raw_count)
+		goto again;
+
+	/*
+	 * Now we have the new raw value and have updated the prev
+	 * timestamp already. We can now calculate the elapsed delta
+	 * (event-)time and add that to the generic event.
+	 *
+	 * Careful, not all hw sign-extends above the physical width
+	 * of the count.
+	 */
+	delta = (new_raw_count << shift) - (prev_raw_count << shift);
+	delta >>= shift;
+
+	local64_add(delta, &event->count);
+
+	return new_raw_count;
+}
+
+static enum hrtimer_restart uncore_pmu_hrtimer(struct hrtimer *hrtimer)
+{
+	struct intel_uncore *uncore;
+	enum hrtimer_restart ret = HRTIMER_RESTART;
+	int bit;
+
+	uncore = container_of(hrtimer, struct intel_uncore, hrtimer);
+	raw_spin_lock(&uncore->lock);
+
+	if (!uncore->n_events) {
+		ret = HRTIMER_NORESTART;
+		goto unlock;
+	}
+
+	intel_uncore_pmu.disable_all();
+
+	for_each_set_bit(bit, uncore->active_mask, X86_PMC_IDX_MAX)
+		uncore_perf_event_update(uncore->events[bit]);
+
+	hrtimer_forward_now(hrtimer, ns_to_ktime(UNCORE_PMU_HRTIMER_INTERVAL));
+
+	intel_uncore_pmu.enable_all();
+unlock:
+	raw_spin_unlock(&uncore->lock);
+	return ret;
+}
+
+static void uncore_pmu_start_hrtimer(struct intel_uncore *uncore)
+{
+	__hrtimer_start_range_ns(&uncore->hrtimer,
+				ns_to_ktime(UNCORE_PMU_HRTIMER_INTERVAL), 0,
+				HRTIMER_MODE_REL_PINNED, 0);
+}
+
+static void uncore_pmu_cancel_hrtimer(struct intel_uncore *uncore)
+{
+	hrtimer_cancel(&uncore->hrtimer);
+}
+
+static void uncore_pmu_init_hrtimer(struct intel_uncore *uncore)
+{
+	hrtimer_init(&uncore->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	uncore->hrtimer.function = uncore_pmu_hrtimer;
+}
+
+static struct pmu uncore_pmu;
+
+static int uncore_pmu_event_init(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (!uncore_pmu_initialized)
+		return -ENOENT;
+
+	if (event->attr.type != uncore_pmu.type)
+		return -ENOENT;
+
+	/*
+	 * Uncore PMU does measure at all privilege level all the time.
+	 * So it doesn't make sense to specify any exclude bits.
+	 */
+	if (event->attr.exclude_user || event->attr.exclude_kernel
+	    || event->attr.exclude_hv || event->attr.exclude_idle)
+		return -ENOENT;
+
+	/* Sampling not supported yet */
+	if (hwc->sample_period)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void uncore_pmu_start(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	u64 now;
+
+	rdmsrl(hwc->event_base, now);
+
+	local64_set(&event->hw.prev_count, now);
+	intel_uncore_pmu.enable(event);
+}
+
+static void uncore_pmu_stop(struct perf_event *event, int flags)
+{
+	intel_uncore_pmu.disable(event);
+	uncore_perf_event_update(event);
+}
+
+static int uncore_pmu_add(struct perf_event *event, int flags)
+{
+	struct cpu_uncore_events *cpuc = &__get_cpu_var(cpu_uncore_events);
+	struct intel_uncore *uncore = cpuc->intel_uncore;
+	int ret = 1;
+	int i;
+
+	raw_spin_lock(&uncore->lock);
+
+	if (event->attr.config == UNCORE_FIXED_EVENT) {
+		i = X86_PMC_IDX_FIXED;
+		goto fixed_event;
+	}
+
+	for (i = 0; i < X86_PMC_IDX_FIXED; i++) {
+fixed_event:
+		if (!uncore->events[i]) {
+			uncore->events[i] = event;
+			uncore->n_events++;
+			event->hw.idx = i;
+			__set_bit(i, uncore->active_mask);
+
+			intel_uncore_pmu.hw_config(event);
+
+			if (flags & PERF_EF_START)
+				uncore_pmu_start(event, flags);
+			ret = 0;
+			break;
+		}
+	}
+
+	if (uncore->n_events == 1) {
+		uncore_pmu_start_hrtimer(uncore);
+		intel_uncore_pmu.enable_all();
+	}
+
+	raw_spin_unlock(&uncore->lock);
+
+	return ret;
+}
+
+static void uncore_pmu_del(struct perf_event *event, int flags)
+{
+	struct cpu_uncore_events *cpuc = &__get_cpu_var(cpu_uncore_events);
+	struct intel_uncore *uncore = cpuc->intel_uncore;
+	int idx = event->hw.idx;
+
+	raw_spin_lock(&uncore->lock);
+
+	if (__test_and_clear_bit(idx, uncore->active_mask)) {
+		uncore->events[idx] = NULL;
+		uncore->n_events--;
+
+		uncore_pmu_stop(event, flags);
+	}
+
+	if (uncore->n_events == 0) {
+		intel_uncore_pmu.disable_all();
+		uncore_pmu_cancel_hrtimer(uncore);
+	}
+
+	raw_spin_unlock(&uncore->lock);
+}
+
+static void uncore_pmu_read(struct perf_event *event)
+{
+	uncore_perf_event_update(event);
+}
+
+#define UNCORE_PMU_NUM_GENERIC_EVENTS 1
+
+static struct pmu_event uncore_events[UNCORE_PMU_NUM_GENERIC_EVENTS] = {
+	{"cycle", 0xffff, },
+};
+
+static void uncore_pmu_add_events(void)
+{
+	perf_pmu_add_events(&uncore_pmu, uncore_events,
+		UNCORE_PMU_NUM_GENERIC_EVENTS);
+}
+
+static struct pmu uncore_pmu = {
+	.task_ctx_nr	= perf_invalid_context,
+	.event_init	= uncore_pmu_event_init,
+	.add		= uncore_pmu_add,
+	.del		= uncore_pmu_del,
+	.start		= uncore_pmu_start,
+	.stop		= uncore_pmu_stop,
+	.read		= uncore_pmu_read,
+	.add_events	= uncore_pmu_add_events,
+};
+
+static struct intel_uncore *alloc_uncore(int cpu, int uncore_id)
+{
+	struct intel_uncore *uncore;
+
+	uncore =
+	    kmalloc_node(sizeof(struct intel_uncore), GFP_KERNEL | __GFP_ZERO,
+			 cpu_to_node(cpu));
+	if (!uncore)
+		return NULL;
+
+	uncore->id = uncore_id;
+	raw_spin_lock_init(&uncore->lock);
+
+	return uncore;
+}
+
+static int uncore_pmu_cpu_prepare(int cpu)
+{
+	struct cpu_uncore_events *cpuc = &per_cpu(cpu_uncore_events, cpu);
+
+	WARN_ON_ONCE(cpuc->intel_uncore);
+
+	cpuc->intel_uncore = alloc_uncore(cpu, -1);
+	if (!cpuc->intel_uncore)
+		return NOTIFY_BAD;
+
+	return NOTIFY_OK;
+}
+
+static void uncore_pmu_cpu_starting(int cpu)
+{
+	struct cpu_uncore_events *cpuc = &per_cpu(cpu_uncore_events, cpu);
+	struct intel_uncore *uncore;
+	int i, uncore_id;
+
+	uncore_id = topology_physical_package_id(cpu);
+	WARN_ON_ONCE(uncore_id == BAD_APICID);
+
+	raw_spin_lock(&intel_uncore_lock);
+
+	for_each_online_cpu(i) {
+		uncore = per_cpu(cpu_uncore_events, i).intel_uncore;
+		if (WARN_ON_ONCE(!uncore))
+			continue;
+
+		if (uncore->id == uncore_id) {
+			kfree(cpuc->intel_uncore);
+			cpuc->intel_uncore = uncore;
+			break;
+		}
+	}
+
+	cpuc->intel_uncore->id = uncore_id;
+	cpuc->intel_uncore->refcnt++;
+	uncore_pmu_init_hrtimer(cpuc->intel_uncore);
+
+	raw_spin_unlock(&intel_uncore_lock);
+}
+
+static void uncore_pmu_cpu_dead(int cpu)
+{
+	struct cpu_uncore_events *cpuhw;
+
+	cpuhw = &per_cpu(cpu_uncore_events, cpu);
+
+	raw_spin_lock(&intel_uncore_lock);
+
+	if (cpuhw->intel_uncore) {
+		struct intel_uncore *uncore = cpuhw->intel_uncore;
+
+		if (uncore->id == -1 || --uncore->refcnt == 0)
+			kfree(uncore);
+
+		cpuhw->intel_uncore = NULL;
+	}
+
+	raw_spin_unlock(&intel_uncore_lock);
+}
+
+static int __cpuinit
+uncore_pmu_notifier(struct notifier_block *self, unsigned long action,
+		    void *hcpu)
+{
+	unsigned int cpu = (long)hcpu;
+	int ret = NOTIFY_OK;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_UP_PREPARE:
+		ret = uncore_pmu_cpu_prepare(cpu);
+		break;
+
+	case CPU_STARTING:
+		uncore_pmu_cpu_starting(cpu);
+		break;
+
+	case CPU_DYING:
+		uncore_pmu_cpu_dead(cpu);
+		break;
+
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+
+static int __init uncore_pmu_init(void)
+{
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+	    boot_cpu_data.x86 != 6)
+		return 0;
+
+	switch (boot_cpu_data.x86_model) {
+	case 26: /* Nehalem */
+	case 30:
+	case 31:
+	case 37: /* Westmere */
+		intel_uncore_pmu = nhm_uncore_pmu;
+		break;
+
+	default:
+		return 0;
+	}
+
+	pr_cont("Performance Events: %s uncore PMU.\n", intel_uncore_pmu.name);
+
+	perf_pmu_register(&uncore_pmu, "uncore", -1);
+	perf_cpu_notifier(uncore_pmu_notifier);
+	uncore_pmu_initialized = true;
+	return 0;
+}
+early_initcall(uncore_pmu_init);
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.h b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
new file mode 100644
index 0000000..431c8b4
--- /dev/null
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
@@ -0,0 +1,53 @@
+#include <linux/perf_event.h>
+#include <linux/kprobes.h>
+#include <linux/hardirq.h>
+#include <linux/slab.h>
+
+#define UNCORE_FIXED_EVENT			0xFFFF
+#define NHM_UNCORE_FIXED_CTR_CTRL_EN		(1ULL << 0)
+
+#define UNCORE_EVENTSEL_EVENT			0x000000FFULL
+#define UNCORE_EVENTSEL_UMASK			0x0000FF00ULL
+#define UNCORE_EVENTSEL_EDGE			(1ULL << 18)
+#define UNCORE_EVENTSEL_ENABLE			(1ULL << 22)
+#define UNCORE_EVENTSEL_INV			(1ULL << 23)
+#define NHM_UNCORE_EVENTSEL_CMASK		0xFF000000ULL
+
+#define NHM_UNCORE_RAW_EVENT_MASK	\
+	(UNCORE_EVENTSEL_EVENT |	\
+	 UNCORE_EVENTSEL_UMASK |	\
+	 UNCORE_EVENTSEL_EDGE  |	\
+	 UNCORE_EVENTSEL_INV   |	\
+	 NHM_UNCORE_EVENTSEL_CMASK)
+
+/* 8 generic counters + 1 fixed counter */
+#define UNCORE_NUM_GENERIC_COUNTERS		8
+#define UNCORE_NUM_FIXED_COUNTERS		1
+#define UNCORE_NUM_COUNTERS			((UNCORE_NUM_GENERIC_COUNTERS) + \
+						(UNCORE_NUM_FIXED_COUNTERS))
+
+struct intel_uncore {
+	int id;			/* uncore id */
+	int refcnt;		/* reference count */
+
+	struct perf_event *events[X86_PMC_IDX_MAX];	/* in counter order */
+	unsigned long active_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
+	int n_events;
+	raw_spinlock_t lock;
+	struct hrtimer hrtimer;
+};
+
+struct cpu_uncore_events {
+	struct intel_uncore *intel_uncore;
+};
+
+struct intel_uncore_pmu {
+	const char	*name;
+	void		(*disable_all)(void);
+	void		(*enable_all)(void);
+	void		(*enable)(struct perf_event *);
+	void		(*disable)(struct perf_event *);
+	void		(*hw_config)(struct perf_event *event);
+	int		cntval_bits;
+	int		cntval_bits_fixed;
+};
-- 
1.7.5.1

Re: [PATCH v2 2/6] perf, x86: Add Intel Nehalem/Westmere uncore pmu

[Lin Ming]

On Fri, Jul 15, 2011 at 10:34 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> +
> +#define UNCORE_PMU_NUM_GENERIC_EVENTS 1
> +
> +static struct pmu_event uncore_events[UNCORE_PMU_NUM_GENERIC_EVENTS] = {
> +       {"cycle", 0xffff, },
> +};

Sorry, hard code 0xffff should be replaced with UNCORE_FIXED_EVENT
The uncore fixed event is used to count uncore cycle.

We can add other general events for uncore pmu later.

Lin Ming

Re: [PATCH v2 2/6] perf, x86: Add Intel Nehalem/Westmere uncore pmu

[Peter Zijlstra]

On Fri, 2011-07-15 at 14:34 +0000, Lin Ming wrote:
> Add Intel Nehalem/Westmere uncore pmu support.
> And also the generic data structure to support uncore pmu.
> 
> Uncore pmu interrupt does not work, so hrtimer is used to pull counters.

s/pull/poll/

> diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> new file mode 100644
> index 0000000..79a501e
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> @@ -0,0 +1,450 @@
> +#include "perf_event_intel_uncore.h"
> +
> +static DEFINE_PER_CPU(struct cpu_uncore_events, cpu_uncore_events);
> +static DEFINE_RAW_SPINLOCK(intel_uncore_lock);
> +
> +static bool uncore_pmu_initialized;
> +static struct intel_uncore_pmu intel_uncore_pmu __read_mostly;
> +
> +/*
> + * Uncore pmu interrupt does not work.
> + * Use hrtimer to pull the counter every 10 seconds.
> + */
> +#define UNCORE_PMU_HRTIMER_INTERVAL (10000000000ULL)

 10 * NSEC_PER_SEC

> +static int uncore_pmu_event_init(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (!uncore_pmu_initialized)
> +		return -ENOENT;
> +
> +	if (event->attr.type != uncore_pmu.type)
> +		return -ENOENT;
> +
> +	/*
> +	 * Uncore PMU does measure at all privilege level all the time.
> +	 * So it doesn't make sense to specify any exclude bits.
> +	 */
> +	if (event->attr.exclude_user || event->attr.exclude_kernel
> +	    || event->attr.exclude_hv || event->attr.exclude_idle)
> +		return -ENOENT;

-EINVAL, the PMU exists and is the right one, we just don't support
this.

> +	/* Sampling not supported yet */
> +	if (hwc->sample_period)
> +		return -EINVAL;
> +
> +	return 0;
> +}

> +static int uncore_pmu_add(struct perf_event *event, int flags)
> +{
> +	struct cpu_uncore_events *cpuc = &__get_cpu_var(cpu_uncore_events);
> +	struct intel_uncore *uncore = cpuc->intel_uncore;
> +	int ret = 1;
> +	int i;
> +
> +	raw_spin_lock(&uncore->lock);
> +
> +	if (event->attr.config == UNCORE_FIXED_EVENT) {
> +		i = X86_PMC_IDX_FIXED;
> +		goto fixed_event;

Can the GP counters also count that event? If so, what happens if I
start 2 of them?

> +	}
> +
> +	for (i = 0; i < X86_PMC_IDX_FIXED; i++) {
> +fixed_event:
> +		if (!uncore->events[i]) {
> +			uncore->events[i] = event;
> +			uncore->n_events++;
> +			event->hw.idx = i;
> +			__set_bit(i, uncore->active_mask);
> +
> +			intel_uncore_pmu.hw_config(event);
> +
> +			if (flags & PERF_EF_START)
> +				uncore_pmu_start(event, flags);
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (uncore->n_events == 1) {
> +		uncore_pmu_start_hrtimer(uncore);
> +		intel_uncore_pmu.enable_all();
> +	}
> +
> +	raw_spin_unlock(&uncore->lock);
> +
> +	return ret;
> +}

uncore is fully symmetric and doesn't have any constraints other than
the fixed counter?

I guess we can start with this, there is still the issue of mapping the
events to a single active cpu in the node, but I guess we can do that a
little later.

Re: [PATCH v2 2/6] perf, x86: Add Intel Nehalem/Westmere uncore pmu

[Lin Ming]

On Mon, 2011-07-18 at 22:20 +0800, Peter Zijlstra wrote:
> On Fri, 2011-07-15 at 14:34 +0000, Lin Ming wrote:
> > Add Intel Nehalem/Westmere uncore pmu support.
> > And also the generic data structure to support uncore pmu.
> > 
> > Uncore pmu interrupt does not work, so hrtimer is used to pull counters.
> 
> s/pull/poll/

Will change.

> 
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> > new file mode 100644
> > index 0000000..79a501e
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
> > @@ -0,0 +1,450 @@
> > +#include "perf_event_intel_uncore.h"
> > +
> > +static DEFINE_PER_CPU(struct cpu_uncore_events, cpu_uncore_events);
> > +static DEFINE_RAW_SPINLOCK(intel_uncore_lock);
> > +
> > +static bool uncore_pmu_initialized;
> > +static struct intel_uncore_pmu intel_uncore_pmu __read_mostly;
> > +
> > +/*
> > + * Uncore pmu interrupt does not work.
> > + * Use hrtimer to pull the counter every 10 seconds.
> > + */
> > +#define UNCORE_PMU_HRTIMER_INTERVAL (10000000000ULL)
> 
>  10 * NSEC_PER_SEC

ok.

> 
> > +static int uncore_pmu_event_init(struct perf_event *event)
> > +{
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	if (!uncore_pmu_initialized)
> > +		return -ENOENT;
> > +
> > +	if (event->attr.type != uncore_pmu.type)
> > +		return -ENOENT;
> > +
> > +	/*
> > +	 * Uncore PMU does measure at all privilege level all the time.
> > +	 * So it doesn't make sense to specify any exclude bits.
> > +	 */
> > +	if (event->attr.exclude_user || event->attr.exclude_kernel
> > +	    || event->attr.exclude_hv || event->attr.exclude_idle)
> > +		return -ENOENT;
> 
> -EINVAL, the PMU exists and is the right one, we just don't support
> this.

ok.

> 
> > +	/* Sampling not supported yet */
> > +	if (hwc->sample_period)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> 
> > +static int uncore_pmu_add(struct perf_event *event, int flags)
> > +{
> > +	struct cpu_uncore_events *cpuc = &__get_cpu_var(cpu_uncore_events);
> > +	struct intel_uncore *uncore = cpuc->intel_uncore;
> > +	int ret = 1;
> > +	int i;
> > +
> > +	raw_spin_lock(&uncore->lock);
> > +
> > +	if (event->attr.config == UNCORE_FIXED_EVENT) {
> > +		i = X86_PMC_IDX_FIXED;
> > +		goto fixed_event;
> 
> Can the GP counters also count that event? If so, what happens if I
> start 2 of them?

For Nehalem, manual says "The fixed-function
uncore counter increments at the rate of the U-clock when enabled."

There is no same event in the Nehalem uncore events list.

For SandyBridge, manual does not tell clearly what the fixed event
counts. But I think it should be similar with Nehalem.

> 
> > +	}
> > +
> > +	for (i = 0; i < X86_PMC_IDX_FIXED; i++) {
> > +fixed_event:
> > +		if (!uncore->events[i]) {
> > +			uncore->events[i] = event;
> > +			uncore->n_events++;
> > +			event->hw.idx = i;
> > +			__set_bit(i, uncore->active_mask);
> > +
> > +			intel_uncore_pmu.hw_config(event);
> > +
> > +			if (flags & PERF_EF_START)
> > +				uncore_pmu_start(event, flags);
> > +			ret = 0;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (uncore->n_events == 1) {
> > +		uncore_pmu_start_hrtimer(uncore);
> > +		intel_uncore_pmu.enable_all();
> > +	}
> > +
> > +	raw_spin_unlock(&uncore->lock);
> > +
> > +	return ret;
> > +}
> 
> uncore is fully symmetric and doesn't have any constraints other than
> the fixed counter?

SandyBridge uncore events 0x0180 and 0x0183 can only use counter 0.

> 
> I guess we can start with this, there is still the issue of mapping the
> events to a single active cpu in the node, but I guess we can do that a
> little later.

Do we really need this mapping with uncore pmu interrupt disabled?

Thanks for comments.

Re: [PATCH v2 2/6] perf, x86: Add Intel Nehalem/Westmere uncore pmu

[Peter Zijlstra]

On Mon, 2011-07-18 at 22:54 +0800, Lin Ming wrote:
> > I guess we can start with this, there is still the issue of mapping the
> > events to a single active cpu in the node, but I guess we can do that a
> > little later.
> 
> Do we really need this mapping with uncore pmu interrupt disabled? 

Yes, because event scheduling is per-cpu. Suppose you've got 8 events, 7
on one cpu, 1 on another, all fits, everything is fine.

Now add an event to the cpu that has 1, at that point the new event
fails to fit on the cpu and that context ends up with nr_events !=
nr_active, and rotation starts.

Now instead of rotating between all 9 events, each getting 8/9 of the
time. It will only rotate the two events, resulting in 7 events that are
always on, and 2 that are on 50-50.

Also, consider cpu-hotplug, for as long as there is at least one core of
the node active, we can count all these events. But now, suppose we
unplug the core with the 7 counters on, they all die.

Anyway, we can fix that later once we get something working.

[PATCH v2 3/6] perf, x86: Add Intel SandyBridge uncore pmu

[Lin Ming]

Add Intel SandyBridge uncore pmu support.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 arch/x86/include/asm/msr-index.h              |   26 +++++++++++-
 arch/x86/kernel/cpu/perf_event_intel_uncore.c |   57 +++++++++++++++++++++++++
 arch/x86/kernel/cpu/perf_event_intel_uncore.h |   11 +++++
 3 files changed, 93 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e66011e..fa0e9e6 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -421,7 +421,7 @@
 #define MSR_CORE_PERF_GLOBAL_CTRL	0x0000038f
 #define MSR_CORE_PERF_GLOBAL_OVF_CTRL	0x00000390
 
-/* Intel Nehalem/Westmere uncore performance counters */
+/* Intel Nehalem/Westmere/SandyBridge uncore performance counters */
 #define MSR_UNCORE_PERF_GLOBAL_CTRL	0x00000391
 #define MSR_UNCORE_FIXED_CTR_CTRL	0x00000394
 #define MSR_UNCORE_FIXED_CTR0		0x00000395
@@ -429,6 +429,30 @@
 #define MSR_NHM_UNCORE_PMC0		0x000003b0
 #define MSR_NHM_UNCORE_PERFEVTSEL0	0x000003c0
 
+#define MSR_SNB_UNCORE_CBO_0_PERFEVTSEL0	0x00000700
+#define MSR_SNB_UNCORE_CBO_0_PERFEVTSEL1	0x00000701
+#define MSR_SNB_UNCORE_CBO_0_UNIT_STATUS	0x00000705
+#define MSR_SNB_UNCORE_CBO_0_PER_CTR0		0x00000706
+#define MSR_SNB_UNCORE_CBO_0_PER_CTR1		0x00000707
+
+#define MSR_SNB_UNCORE_CBO_1_PERFEVTSEL0	0x00000710
+#define MSR_SNB_UNCORE_CBO_1_PERFEVTSEL1	0x00000711
+#define MSR_SNB_UNCORE_CBO_1_UNIT_STATUS	0x00000715
+#define MSR_SNB_UNCORE_CBO_1_PER_CTR0		0x00000716
+#define MSR_SNB_UNCORE_CBO_1_PER_CTR1		0x00000717
+
+#define MSR_SNB_UNCORE_CBO_2_PERFEVTSEL0	0x00000720
+#define MSR_SNB_UNCORE_CBO_2_PERFEVTSEL1	0x00000721
+#define MSR_SNB_UNCORE_CBO_2_UNIT_STATUS	0x00000725
+#define MSR_SNB_UNCORE_CBO_2_PER_CTR0		0x00000726
+#define MSR_SNB_UNCORE_CBO_2_PER_CTR1		0x00000727
+
+#define MSR_SNB_UNCORE_CBO_3_PERFEVTSEL0	0x00000730
+#define MSR_SNB_UNCORE_CBO_3_PERFEVTSEL1	0x00000731
+#define MSR_SNB_UNCORE_CBO_3_UNIT_STATUS	0x00000735
+#define MSR_SNB_UNCORE_CBO_3_PER_CTR0		0x00000736
+#define MSR_SNB_UNCORE_CBO_3_PER_CTR1		0x00000737
+
 /* Geode defined MSRs */
 #define MSR_GEODE_BUSCONT_CONF0		0x00001900
 
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
index 79a501e..1100589 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -98,6 +98,59 @@ static __initconst const struct intel_uncore_pmu nhm_uncore_pmu = {
 	.cntval_bits_fixed	= 48,
 };
 
+/* SandyBridge uncore pmu */
+
+static struct uncore_config {
+	unsigned long config_base;
+	unsigned long event_base;
+} snb_uncore_configs[UNCORE_NUM_GENERIC_COUNTERS] = {
+	{MSR_SNB_UNCORE_CBO_0_PERFEVTSEL0, MSR_SNB_UNCORE_CBO_0_PER_CTR0},
+	{MSR_SNB_UNCORE_CBO_0_PERFEVTSEL1, MSR_SNB_UNCORE_CBO_0_PER_CTR1},
+	{MSR_SNB_UNCORE_CBO_1_PERFEVTSEL0, MSR_SNB_UNCORE_CBO_1_PER_CTR0},
+	{MSR_SNB_UNCORE_CBO_1_PERFEVTSEL1, MSR_SNB_UNCORE_CBO_1_PER_CTR1},
+	{MSR_SNB_UNCORE_CBO_2_PERFEVTSEL0, MSR_SNB_UNCORE_CBO_2_PER_CTR0},
+	{MSR_SNB_UNCORE_CBO_2_PERFEVTSEL1, MSR_SNB_UNCORE_CBO_2_PER_CTR1},
+	{MSR_SNB_UNCORE_CBO_3_PERFEVTSEL0, MSR_SNB_UNCORE_CBO_3_PER_CTR0},
+	{MSR_SNB_UNCORE_CBO_3_PERFEVTSEL1, MSR_SNB_UNCORE_CBO_3_PER_CTR1},
+};
+
+static void snb_uncore_pmu_enable_all(void)
+{
+	wrmsrl(MSR_UNCORE_PERF_GLOBAL_CTRL,
+		SNB_UNCORE_PERF_GLOBAL_CTRL_EN);
+}
+
+static void snb_uncore_pmu_hw_config(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	int i = hwc->idx;
+
+	if (event->hw.idx == X86_PMC_IDX_FIXED) {
+		uncore_fixed_hw_config(event);
+		return;
+	}
+
+	hwc->config = event->attr.config & SNB_UNCORE_RAW_EVENT_MASK;
+	hwc->config_base = snb_uncore_configs[i].config_base;
+	hwc->event_base = snb_uncore_configs[i].event_base;
+}
+
+static void snb_uncore_pmu_enable_event(struct perf_event *event)
+{
+	uncore_pmu_enable_event(event, SNB_UNCORE_FIXED_CTR_CTRL_EN);
+}
+
+static __initconst const struct intel_uncore_pmu snb_uncore_pmu = {
+	.name			= "SandyBridge",
+	.disable_all		= uncore_pmu_disable_all,
+	.enable_all		= snb_uncore_pmu_enable_all,
+	.enable			= snb_uncore_pmu_enable_event,
+	.disable		= uncore_pmu_disable_event,
+	.hw_config		= snb_uncore_pmu_hw_config,
+	.cntval_bits		= 44,
+	.cntval_bits_fixed	= 48,
+};
+
 static u64 uncore_perf_event_update(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -436,6 +489,10 @@ static int __init uncore_pmu_init(void)
 		intel_uncore_pmu = nhm_uncore_pmu;
 		break;
 
+	case 42: /* SandyBridge */
+		intel_uncore_pmu = snb_uncore_pmu;
+		break;
+
 	default:
 		return 0;
 	}
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.h b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
index 431c8b4..c7392aa 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.h
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
@@ -5,6 +5,9 @@
 
 #define UNCORE_FIXED_EVENT			0xFFFF
 #define NHM_UNCORE_FIXED_CTR_CTRL_EN		(1ULL << 0)
+#define SNB_UNCORE_FIXED_CTR_CTRL_EN		(1ULL << 22)
+
+#define SNB_UNCORE_PERF_GLOBAL_CTRL_EN		(1ULL << 29)
 
 #define UNCORE_EVENTSEL_EVENT			0x000000FFULL
 #define UNCORE_EVENTSEL_UMASK			0x0000FF00ULL
@@ -12,6 +15,7 @@
 #define UNCORE_EVENTSEL_ENABLE			(1ULL << 22)
 #define UNCORE_EVENTSEL_INV			(1ULL << 23)
 #define NHM_UNCORE_EVENTSEL_CMASK		0xFF000000ULL
+#define SNB_UNCORE_EVENTSEL_CMASK		0x1F000000ULL
 
 #define NHM_UNCORE_RAW_EVENT_MASK	\
 	(UNCORE_EVENTSEL_EVENT |	\
@@ -20,6 +24,13 @@
 	 UNCORE_EVENTSEL_INV   |	\
 	 NHM_UNCORE_EVENTSEL_CMASK)
 
+#define SNB_UNCORE_RAW_EVENT_MASK	\
+	(UNCORE_EVENTSEL_EVENT |	\
+	 UNCORE_EVENTSEL_UMASK |	\
+	 UNCORE_EVENTSEL_EDGE  |	\
+	 UNCORE_EVENTSEL_INV   |	\
+	 SNB_UNCORE_EVENTSEL_CMASK)
+
 /* 8 generic counters + 1 fixed counter */
 #define UNCORE_NUM_GENERIC_COUNTERS		8
 #define UNCORE_NUM_FIXED_COUNTERS		1
-- 
1.7.5.1

[PATCH v2 4/6] perf: Remove perf_event_attr::type check

[Lin Ming]

PMU type id can be allocated dynamically, so perf_event_attr::type check
when copying attribute from userspace to kernel is not valid.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 kernel/events/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9f4f463..7bd8d5d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5928,13 +5928,6 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	if (ret)
 		return -EFAULT;
 
-	/*
-	 * If the type exists, the corresponding creation will verify
-	 * the attr->config.
-	 */
-	if (attr->type >= PERF_TYPE_MAX)
-		return -EINVAL;
-
 	if (attr->__reserved_1)
 		return -EINVAL;
 
-- 
1.7.5.1

[PATCH v2 5/6] perf tool: Allow system-wide 'perf stat' without 'command'

[Lin Ming]

There are package level counters, for example Intel uncore counters,
can't be limited to process. So we should allow 'perf stat' to run without 'command'.

$ sudo perf stat -a -C 0 -e uncore:cycle
^C
 Performance counter stats for 'CPU 0':

        56,547,314 uncore:cycle

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 tools/perf/builtin-stat.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 1d08c80..84546ff 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -958,7 +958,14 @@ static void print_stat(int argc, const char **argv)
 	if (!csv_output) {
 		fprintf(stderr, "\n");
 		fprintf(stderr, " Performance counter stats for ");
-		if(target_pid == -1 && target_tid == -1) {
+		if (system_wide) {
+			fprintf(stderr, "\'CPU ");
+			for (i = 0; i < evsel_list->cpus->nr; i++) {
+				fprintf(stderr, "%d", evsel_list->cpus->map[i]);
+				if (i < evsel_list->cpus->nr - 1)
+					fprintf(stderr, ",");
+			}
+		} else if (target_pid == -1 && target_tid == -1) {
 			fprintf(stderr, "\'%s", argv[0]);
 			for (i = 1; i < argc; i++)
 				fprintf(stderr, " %s", argv[i]);
@@ -1166,7 +1173,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	} else if (big_num_opt == 0) /* User passed --no-big-num */
 		big_num = false;
 
-	if (!argc && target_pid == -1 && target_tid == -1)
+	if (!system_wide && !argc && target_pid == -1 && target_tid == -1)
 		usage_with_options(stat_usage, options);
 	if (run_count <= 0)
 		usage_with_options(stat_usage, options);
-- 
1.7.5.1

[PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Lin Ming]

PMU can export general events to sysfs, for example,

/sys/bus/event_source/devices/uncore/events
└── cycle

Then specify the event as <pmu>:<event>,

$ sudo perf stat -a -C 0 -e uncore:cycle
^C
 Performance counter stats for 'CPU 0':

        56,547,314 uncore:cycle

Raw event can be specified as <pmu>:rXXXX

$ sudo perf stat -a -C 0 -e uncore:r0101
^C
 Performance counter stats for 'CPU 0':

             8,504 uncore:r0101

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 tools/perf/util/parse-events.c |   84 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 41982c3..2de52f9 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -684,7 +684,7 @@ parse_symbolic_event(const char **strp, struct perf_event_attr *attr)
 }
 
 static enum event_result
-parse_raw_event(const char **strp, struct perf_event_attr *attr)
+parse_raw_config(const char **strp, struct perf_event_attr *attr)
 {
 	const char *str = *strp;
 	u64 config;
@@ -695,7 +695,6 @@ parse_raw_event(const char **strp, struct perf_event_attr *attr)
 	n = hex2u64(str + 1, &config);
 	if (n > 0) {
 		*strp = str + n + 1;
-		attr->type = PERF_TYPE_RAW;
 		attr->config = config;
 		return EVT_HANDLED;
 	}
@@ -703,6 +702,83 @@ parse_raw_event(const char **strp, struct perf_event_attr *attr)
 }
 
 static enum event_result
+parse_raw_event(const char **strp, struct perf_event_attr *attr)
+{
+	if (parse_raw_config(strp, attr) != EVT_HANDLED)
+		return EVT_FAILED;
+
+	attr->type = PERF_TYPE_RAW;
+	return EVT_HANDLED;
+}
+
+#define EVENT_SOURCE_DIR "/sys/bus/event_source/devices"
+
+static u64 read_sysfs_entry(const char *path)
+{
+	char buf[19];
+	int fd;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return -1;
+
+	if (read(fd, buf, sizeof(buf)) < 0) {
+		close(fd);
+		return -1;
+	}
+
+	close(fd);
+	return atoll(buf);
+}
+
+static u64 get_pmu_type(const char *pmu_name)
+{
+	char evt_path[MAXPATHLEN];
+
+	snprintf(evt_path, MAXPATHLEN, "%s/%s/type", EVENT_SOURCE_DIR,
+		 pmu_name);
+
+	return read_sysfs_entry(evt_path);
+}
+
+static u64 get_pmu_event_config(const char *pmu_name, const char *evt_name)
+{
+	char evt_path[MAXPATHLEN];
+
+	snprintf(evt_path, MAXPATHLEN, "%s/%s/events/%s", EVENT_SOURCE_DIR,
+		 pmu_name, evt_name);
+
+	return read_sysfs_entry(evt_path);
+}
+
+static enum event_result
+parse_sysfs_event(const char **strp, struct perf_event_attr *attr)
+{
+	char *pmu_name, *evt_name;
+	u64 type, config;
+
+	pmu_name = strchr(*strp, ':');
+	if (!pmu_name)
+		return EVT_FAILED;
+	pmu_name = strndup(*strp, pmu_name - *strp);
+	type = get_pmu_type(pmu_name);
+	if ((int)type < 0)
+		return EVT_FAILED;
+	attr->type = type;
+
+	evt_name = strchr(*strp, ':') + 1;
+	config = get_pmu_event_config(pmu_name, evt_name);
+	*strp += strlen(pmu_name) + 1; /* + 1 for the ':' */
+
+	if ((int)config < 0)
+		return parse_raw_config(strp, attr);
+
+	attr->config = config;
+	*strp += strlen(evt_name);
+	return EVT_HANDLED;
+}
+
+static enum event_result
 parse_numeric_event(const char **strp, struct perf_event_attr *attr)
 {
 	const char *str = *strp;
@@ -783,6 +859,10 @@ parse_event_symbols(const struct option *opt, const char **str,
 {
 	enum event_result ret;
 
+	ret = parse_sysfs_event(str, attr);
+	if (ret != EVT_FAILED)
+		goto modifier;
+
 	ret = parse_tracepoint_event(opt, str, attr);
 	if (ret != EVT_FAILED)
 		goto modifier;
-- 
1.7.5.1

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Stephane Eranian]

Hi,

On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> PMU can export general events to sysfs, for example,
>
> /sys/bus/event_source/devices/uncore/events
> └── cycle
>
> Then specify the event as <pmu>:<event>,
>
> $ sudo perf stat -a -C 0 -e uncore:cycle

I think this event syntax should be adjusted a bit.

How would the tool differentiate:
       perf stat -e uncore:cycle
form:
      perf stat -e cycle:u

It would have to scan sysfs for a 'cycle' PMU and conclude
there is none,  then resolve the 'cycle' event name. And if
you're unlucky and you have a event name that matches
the PMU name, you get into troubles.

I think, one could instead do:

    perf stat -e uncore::cycle:k

That way, by virtue of the '::' separator, the tool would know
that it needs to first look into sysfs for an 'uncore' PMU, then
it needs to look for the 'cycle' event.

I also use the '::' notation in libpfm4 to separate the PMU model
form the event+umask+modifiers.

I also suspect that with this sysfs interface for PMU models, you
would simply add a number to differentiate each instance of a PMU.
So for GPU, you would do:
    perf stat -e  gfx0::cycles

Is that right?

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Lin Ming]

On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
> Hi,
> 
> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> > PMU can export general events to sysfs, for example,
> >
> > /sys/bus/event_source/devices/uncore/events
> > └── cycle
> >
> > Then specify the event as <pmu>:<event>,
> >
> > $ sudo perf stat -a -C 0 -e uncore:cycle
> 
> I think this event syntax should be adjusted a bit.
> 
> How would the tool differentiate:
>        perf stat -e uncore:cycle
> form:
>       perf stat -e cycle:u
> 
> It would have to scan sysfs for a 'cycle' PMU and conclude
> there is none,  then resolve the 'cycle' event name. And if
> you're unlucky and you have a event name that matches
> the PMU name, you get into troubles.
> 
> I think, one could instead do:
> 
>     perf stat -e uncore::cycle:k
> 
> That way, by virtue of the '::' separator, the tool would know
> that it needs to first look into sysfs for an 'uncore' PMU, then
> it needs to look for the 'cycle' event.

Yes, I like this '::' separator too.
Will update to use it.

> 
> I also use the '::' notation in libpfm4 to separate the PMU model
> form the event+umask+modifiers.
> 
> I also suspect that with this sysfs interface for PMU models, you
> would simply add a number to differentiate each instance of a PMU.
> So for GPU, you would do:
>     perf stat -e  gfx0::cycles
> 
> Is that right?

A number or other thing is OK.
 
int perf_pmu_register(struct pmu *pmu, char *name, int type)
will be called to register a PMU.

So I think any name that can differentiate each instance is OK.

Adding a number looks like the easiest way.

Thanks,
Lin Ming

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Stephane Eranian]

On Sat, Aug 6, 2011 at 4:38 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
>> Hi,
>>
>> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > PMU can export general events to sysfs, for example,
>> >
>> > /sys/bus/event_source/devices/uncore/events
>> > └── cycle
>> >
>> > Then specify the event as <pmu>:<event>,
>> >
>> > $ sudo perf stat -a -C 0 -e uncore:cycle
>>
>> I think this event syntax should be adjusted a bit.
>>
>> How would the tool differentiate:
>>        perf stat -e uncore:cycle
>> form:
>>       perf stat -e cycle:u
>>
>> It would have to scan sysfs for a 'cycle' PMU and conclude
>> there is none,  then resolve the 'cycle' event name. And if
>> you're unlucky and you have a event name that matches
>> the PMU name, you get into troubles.
>>
>> I think, one could instead do:
>>
>>     perf stat -e uncore::cycle:k
>>
>> That way, by virtue of the '::' separator, the tool would know
>> that it needs to first look into sysfs for an 'uncore' PMU, then
>> it needs to look for the 'cycle' event.
>
> Yes, I like this '::' separator too.
> Will update to use it.
>
>>
>> I also use the '::' notation in libpfm4 to separate the PMU model
>> form the event+umask+modifiers.
>>
>> I also suspect that with this sysfs interface for PMU models, you
>> would simply add a number to differentiate each instance of a PMU.
>> So for GPU, you would do:
>>     perf stat -e  gfx0::cycles
>>
>> Is that right?
>
> A number or other thing is OK.
>
> int perf_pmu_register(struct pmu *pmu, char *name, int type)
> will be called to register a PMU.
>
> So I think any name that can differentiate each instance is OK.
>
> Adding a number looks like the easiest way.
>
Well, there is something I am still missing here.

Based on the current patch, it seems that each instance
of a PMU needs to register to get an ID and an entry in
sysfs.

Suppose you have a system with two graphics cards. Then,
you would need two IDs and two entries in sysfs to correctly
name each gfx card.

That means that the kernel would have to iterate over each instance
of a PMU and create a name for it, e.g., something like:
   for_each_gfx_card(i) {
       sprintf(name, "gfx%d", i);
      register_pmu(&pmu, name);
  }

Is that what you are proposing?

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Lin Ming]

On Mon, 2011-08-08 at 07:47 +0800, Stephane Eranian wrote:
> On Sat, Aug 6, 2011 at 4:38 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> > On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
> >> Hi,
> >>
> >> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
> >> > PMU can export general events to sysfs, for example,
> >> >
> >> > /sys/bus/event_source/devices/uncore/events
> >> > └── cycle
> >> >
> >> > Then specify the event as <pmu>:<event>,
> >> >
> >> > $ sudo perf stat -a -C 0 -e uncore:cycle
> >>
> >> I think this event syntax should be adjusted a bit.
> >>
> >> How would the tool differentiate:
> >>        perf stat -e uncore:cycle
> >> form:
> >>       perf stat -e cycle:u
> >>
> >> It would have to scan sysfs for a 'cycle' PMU and conclude
> >> there is none,  then resolve the 'cycle' event name. And if
> >> you're unlucky and you have a event name that matches
> >> the PMU name, you get into troubles.
> >>
> >> I think, one could instead do:
> >>
> >>     perf stat -e uncore::cycle:k
> >>
> >> That way, by virtue of the '::' separator, the tool would know
> >> that it needs to first look into sysfs for an 'uncore' PMU, then
> >> it needs to look for the 'cycle' event.
> >
> > Yes, I like this '::' separator too.
> > Will update to use it.
> >
> >>
> >> I also use the '::' notation in libpfm4 to separate the PMU model
> >> form the event+umask+modifiers.
> >>
> >> I also suspect that with this sysfs interface for PMU models, you
> >> would simply add a number to differentiate each instance of a PMU.
> >> So for GPU, you would do:
> >>     perf stat -e  gfx0::cycles
> >>
> >> Is that right?
> >
> > A number or other thing is OK.
> >
> > int perf_pmu_register(struct pmu *pmu, char *name, int type)
> > will be called to register a PMU.
> >
> > So I think any name that can differentiate each instance is OK.
> >
> > Adding a number looks like the easiest way.
> >
> Well, there is something I am still missing here.
> 
> Based on the current patch, it seems that each instance
> of a PMU needs to register to get an ID and an entry in
> sysfs.
> 
> Suppose you have a system with two graphics cards. Then,
> you would need two IDs and two entries in sysfs to correctly
> name each gfx card.
> 
> That means that the kernel would have to iterate over each instance
> of a PMU and create a name for it, e.g., something like:
>    for_each_gfx_card(i) {
>        sprintf(name, "gfx%d", i);
>       register_pmu(&pmu, name);
>   }
> 
> Is that what you are proposing?

Think this more closely. My previous reply was not correct.

We only need to register one pmu with two same graphics cards.
Then we can overload pid argument of sys_perf_event_open() to
differentiate each instance of graphic card.

Just like perf cgroup does.

Lin Ming

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Stephane Eranian]

On Sun, Aug 7, 2011 at 6:08 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Mon, 2011-08-08 at 07:47 +0800, Stephane Eranian wrote:
>> On Sat, Aug 6, 2011 at 4:38 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
>> >> Hi,
>> >>
>> >> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> >> > PMU can export general events to sysfs, for example,
>> >> >
>> >> > /sys/bus/event_source/devices/uncore/events
>> >> > └── cycle
>> >> >
>> >> > Then specify the event as <pmu>:<event>,
>> >> >
>> >> > $ sudo perf stat -a -C 0 -e uncore:cycle
>> >>
>> >> I think this event syntax should be adjusted a bit.
>> >>
>> >> How would the tool differentiate:
>> >>        perf stat -e uncore:cycle
>> >> form:
>> >>       perf stat -e cycle:u
>> >>
>> >> It would have to scan sysfs for a 'cycle' PMU and conclude
>> >> there is none,  then resolve the 'cycle' event name. And if
>> >> you're unlucky and you have a event name that matches
>> >> the PMU name, you get into troubles.
>> >>
>> >> I think, one could instead do:
>> >>
>> >>     perf stat -e uncore::cycle:k
>> >>
>> >> That way, by virtue of the '::' separator, the tool would know
>> >> that it needs to first look into sysfs for an 'uncore' PMU, then
>> >> it needs to look for the 'cycle' event.
>> >
>> > Yes, I like this '::' separator too.
>> > Will update to use it.
>> >
>> >>
>> >> I also use the '::' notation in libpfm4 to separate the PMU model
>> >> form the event+umask+modifiers.
>> >>
>> >> I also suspect that with this sysfs interface for PMU models, you
>> >> would simply add a number to differentiate each instance of a PMU.
>> >> So for GPU, you would do:
>> >>     perf stat -e  gfx0::cycles
>> >>
>> >> Is that right?
>> >
>> > A number or other thing is OK.
>> >
>> > int perf_pmu_register(struct pmu *pmu, char *name, int type)
>> > will be called to register a PMU.
>> >
>> > So I think any name that can differentiate each instance is OK.
>> >
>> > Adding a number looks like the easiest way.
>> >
>> Well, there is something I am still missing here.
>>
>> Based on the current patch, it seems that each instance
>> of a PMU needs to register to get an ID and an entry in
>> sysfs.
>>
>> Suppose you have a system with two graphics cards. Then,
>> you would need two IDs and two entries in sysfs to correctly
>> name each gfx card.
>>
>> That means that the kernel would have to iterate over each instance
>> of a PMU and create a name for it, e.g., something like:
>>    for_each_gfx_card(i) {
>>        sprintf(name, "gfx%d", i);
>>       register_pmu(&pmu, name);
>>   }
>>
>> Is that what you are proposing?
>
> Think this more closely. My previous reply was not correct.
>
> We only need to register one pmu with two same graphics cards.
> Then we can overload pid argument of sys_perf_event_open() to
> differentiate each instance of graphic card.
>
Ok, then you are predicting that no other PMU will ever be useful
in per-thread mode.

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Peter Zijlstra]

On Mon, 2011-08-08 at 09:08 +0800, Lin Ming wrote:
> 
> We only need to register one pmu with two same graphics cards.
> Then we can overload pid argument of sys_perf_event_open() to
> differentiate each instance of graphic card.
> 
> Just like perf cgroup does. 

I don't like that much at all.. For one it will make it impossible to
put the pmu device in the right sysfs topology.

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Lin Ming]

On Mon, 2011-08-08 at 16:48 +0800, Peter Zijlstra wrote:
> On Mon, 2011-08-08 at 09:08 +0800, Lin Ming wrote:
> > 
> > We only need to register one pmu with two same graphics cards.
> > Then we can overload pid argument of sys_perf_event_open() to
> > differentiate each instance of graphic card.
> > 
> > Just like perf cgroup does. 
> 
> I don't like that much at all.. For one it will make it impossible to
> put the pmu device in the right sysfs topology.

So do you prefer adding a number to gpu name, as Stephane mentioned?

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Peter Zijlstra]

On Mon, 2011-08-08 at 16:57 +0800, Lin Ming wrote:
> On Mon, 2011-08-08 at 16:48 +0800, Peter Zijlstra wrote:
> > On Mon, 2011-08-08 at 09:08 +0800, Lin Ming wrote:
> > > 
> > > We only need to register one pmu with two same graphics cards.
> > > Then we can overload pid argument of sys_perf_event_open() to
> > > differentiate each instance of graphic card.
> > > 
> > > Just like perf cgroup does. 
> > 
> > I don't like that much at all.. For one it will make it impossible to
> > put the pmu device in the right sysfs topology.
> 
> So do you prefer adding a number to gpu name, as Stephane mentioned?

Not quite having read back that far (reading my email in date-reverse
manner this morning), yeah. That allows links from /sys/bus/pci/... back
to the pmu device.

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Stephane Eranian]

Lin,

On Sun, Aug 7, 2011 at 6:08 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Mon, 2011-08-08 at 07:47 +0800, Stephane Eranian wrote:
>> On Sat, Aug 6, 2011 at 4:38 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>> > On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
>> >> Hi,
>> >>
>> >> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>> >> > PMU can export general events to sysfs, for example,
>> >> >
>> >> > /sys/bus/event_source/devices/uncore/events
>> >> > └── cycle
>> >> >
>> >> > Then specify the event as <pmu>:<event>,
>> >> >
>> >> > $ sudo perf stat -a -C 0 -e uncore:cycle
>> >>
>> >> I think this event syntax should be adjusted a bit.
>> >>
>> >> How would the tool differentiate:
>> >>        perf stat -e uncore:cycle
>> >> form:
>> >>       perf stat -e cycle:u
>> >>
>> >> It would have to scan sysfs for a 'cycle' PMU and conclude
>> >> there is none,  then resolve the 'cycle' event name. And if
>> >> you're unlucky and you have a event name that matches
>> >> the PMU name, you get into troubles.
>> >>
>> >> I think, one could instead do:
>> >>
>> >>     perf stat -e uncore::cycle:k
>> >>
>> >> That way, by virtue of the '::' separator, the tool would know
>> >> that it needs to first look into sysfs for an 'uncore' PMU, then
>> >> it needs to look for the 'cycle' event.
>> >
>> > Yes, I like this '::' separator too.
>> > Will update to use it.
>> >
>> >>
>> >> I also use the '::' notation in libpfm4 to separate the PMU model
>> >> form the event+umask+modifiers.
>> >>
>> >> I also suspect that with this sysfs interface for PMU models, you
>> >> would simply add a number to differentiate each instance of a PMU.
>> >> So for GPU, you would do:
>> >>     perf stat -e  gfx0::cycles
>> >>
>> >> Is that right?
>> >
>> > A number or other thing is OK.
>> >
>> > int perf_pmu_register(struct pmu *pmu, char *name, int type)
>> > will be called to register a PMU.
>> >
>> > So I think any name that can differentiate each instance is OK.
>> >
>> > Adding a number looks like the easiest way.
>> >
>> Well, there is something I am still missing here.
>>
>> Based on the current patch, it seems that each instance
>> of a PMU needs to register to get an ID and an entry in
>> sysfs.
>>
>> Suppose you have a system with two graphics cards. Then,
>> you would need two IDs and two entries in sysfs to correctly
>> name each gfx card.
>>
>> That means that the kernel would have to iterate over each instance
>> of a PMU and create a name for it, e.g., something like:
>>    for_each_gfx_card(i) {
>>        sprintf(name, "gfx%d", i);
>>       register_pmu(&pmu, name);
>>   }
>>
>> Is that what you are proposing?
>
> Think this more closely. My previous reply was not correct.
>
> We only need to register one pmu with two same graphics cards.
> Then we can overload pid argument of sys_perf_event_open() to
> differentiate each instance of graphic card.
>
> Just like perf cgroup does.
>
Some more thoughts on that.

Cgroup is very specific. It can only work an an extension of system-wide mode.
Thus, we know we can safely overload the PID argument with the cgroup fd. But
then, we also need to tell the kernel that PID is a cgroup fd and for
that we use
a flag.

I don't think you can do that with your PMU proposal.

You can pass the PMU class in attr.config, but if you pass the PMU instance
in PID, it means you predict that there will never be another PMU
where per-thread
mode will make sense. But even if that were to be true, you'd have to
still have to
pass an extra flag to denote that you are using a extended PMU with an instance
in PID. Or the alternative it to rely on PERF_TYPE_MAX to detect a dynamically
registered PMU. It's not too pretty. This whole thing makes me wonder
what perf_type_id
is meant for then....

Re: [PATCH v2 6/6] perf tool: Parse general/raw events from sysfs

[Corey Ashford]

On 08/11/2011 03:38 PM, Stephane Eranian wrote:
> Lin,
> 
> On Sun, Aug 7, 2011 at 6:08 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>> On Mon, 2011-08-08 at 07:47 +0800, Stephane Eranian wrote:
>>> On Sat, Aug 6, 2011 at 4:38 PM, Lin Ming <ming.m.lin@intel.com> wrote:
>>>> On Sun, 2011-08-07 at 04:10 +0800, Stephane Eranian wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Jul 15, 2011 at 7:35 AM, Lin Ming <ming.m.lin@intel.com> wrote:
>>>>>> PMU can export general events to sysfs, for example,
>>>>>>
>>>>>> /sys/bus/event_source/devices/uncore/events
>>>>>> └── cycle
>>>>>>
>>>>>> Then specify the event as <pmu>:<event>,
>>>>>>
>>>>>> $ sudo perf stat -a -C 0 -e uncore:cycle
>>>>>
>>>>> I think this event syntax should be adjusted a bit.
>>>>>
>>>>> How would the tool differentiate:
>>>>>        perf stat -e uncore:cycle
>>>>> form:
>>>>>       perf stat -e cycle:u
>>>>>
>>>>> It would have to scan sysfs for a 'cycle' PMU and conclude
>>>>> there is none,  then resolve the 'cycle' event name. And if
>>>>> you're unlucky and you have a event name that matches
>>>>> the PMU name, you get into troubles.
>>>>>
>>>>> I think, one could instead do:
>>>>>
>>>>>     perf stat -e uncore::cycle:k
>>>>>
>>>>> That way, by virtue of the '::' separator, the tool would know
>>>>> that it needs to first look into sysfs for an 'uncore' PMU, then
>>>>> it needs to look for the 'cycle' event.
>>>>
>>>> Yes, I like this '::' separator too.
>>>> Will update to use it.
>>>>
>>>>>
>>>>> I also use the '::' notation in libpfm4 to separate the PMU model
>>>>> form the event+umask+modifiers.
>>>>>
>>>>> I also suspect that with this sysfs interface for PMU models, you
>>>>> would simply add a number to differentiate each instance of a PMU.
>>>>> So for GPU, you would do:
>>>>>     perf stat -e  gfx0::cycles
>>>>>
>>>>> Is that right?
>>>>
>>>> A number or other thing is OK.
>>>>
>>>> int perf_pmu_register(struct pmu *pmu, char *name, int type)
>>>> will be called to register a PMU.
>>>>
>>>> So I think any name that can differentiate each instance is OK.
>>>>
>>>> Adding a number looks like the easiest way.
>>>>
>>> Well, there is something I am still missing here.
>>>
>>> Based on the current patch, it seems that each instance
>>> of a PMU needs to register to get an ID and an entry in
>>> sysfs.
>>>
>>> Suppose you have a system with two graphics cards. Then,
>>> you would need two IDs and two entries in sysfs to correctly
>>> name each gfx card.
>>>
>>> That means that the kernel would have to iterate over each instance
>>> of a PMU and create a name for it, e.g., something like:
>>>    for_each_gfx_card(i) {
>>>        sprintf(name, "gfx%d", i);
>>>       register_pmu(&pmu, name);
>>>   }
>>>
>>> Is that what you are proposing?
>>
>> Think this more closely. My previous reply was not correct.
>>
>> We only need to register one pmu with two same graphics cards.
>> Then we can overload pid argument of sys_perf_event_open() to
>> differentiate each instance of graphic card.
>>
>> Just like perf cgroup does.
>>
> Some more thoughts on that.
> 
> Cgroup is very specific. It can only work an an extension of system-wide mode.
> Thus, we know we can safely overload the PID argument with the cgroup fd. But
> then, we also need to tell the kernel that PID is a cgroup fd and for
> that we use
> a flag.
> 
> I don't think you can do that with your PMU proposal.
> 
> You can pass the PMU class in attr.config, but if you pass the PMU instance
> in PID, it means you predict that there will never be another PMU
> where per-thread
> mode will make sense. But even if that were to be true, you'd have to
> still have to
> pass an extra flag to denote that you are using a extended PMU with an instance
> in PID. Or the alternative it to rely on PERF_TYPE_MAX to detect a dynamically
> registered PMU. It's not too pretty. This whole thing makes me wonder
> what perf_type_id
> is meant for then....

I may be way off base here, since I've been out of the loop for awhile....

Wouldn't it be better to specify the path of the PMU in sysfs as part of
the event name?

For example, if there are two GPUs, you specify the path of the GPU's
PMU that you want.

I don't know how that would look exactly in sysfs, but suppose the path
to the GPU PMU is

/sys/class/gpu/devices/gpu00/pmu

The event specifier to perf would be

gpu/gpu00::event1

(Note, the path prefix and "devices" have been compressed out to make
the PMU specifier more convenient to type)

perf would read the id of the PMU from the id "file" in the gpu00/pmu
directory and then pass that as the attr type field.

This way, you don't have to give a separate numbering system to the GPUs.

- Corey

Re: [PATCH v2 0/6] perf: Intel uncore pmu counting support

[Stephane Eranian]

Hi,

So I finally got a bit of time to test the Ming's uncore PMU patch series
on Intel NHM and SNB. I found several problems which I fixed and I also
made some small improvements. With those fixes + improvements, the
NHM/WSM patches look okay.

OTOH, the SNB patch needs more work. The SNB uncore PMU is more
complicated than what the code supports today based on the info in Vol3b.
For instance, SNB uncore events have constraints, therefore you need a
proper scheduling function.

I will post my patch in separate email thread.



On Fri, Jul 15, 2011 at 4:34 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> Hi, all
>
> This is the v2 patches to support uncore pmu counting.
> The most important change is to use hrtimer to pull uncore counters
> to handle overflow, because uncore pmu interrupt does not work.
>
> v2 -> v1:
> - Use hrtimer to pull counters
> - Add interface to add general events to sysfs
> - Move MSRs to msr-index.h
> - Add uncore fixed counter
> - Allow system-wide 'perf stat' without 'command'
>
> Comments are appreciated.
>
> Thanks,
> Lin Ming
>
>