Patchwork [3/4] perf/POWER7: Make event translations available in sysfs

Jiri Olsa [jolsa@redhat.com] wrote:
| On Wed, Nov 07, 2012 at 11:19:28AM -0800, Sukadev Bhattiprolu wrote:
| 
| SNIP
| 
| > +struct perf_pmu_events_attr {
| > +	struct device_attribute attr;
| > +	u64 id;
| > +};
| > +
| > +extern ssize_t power_events_sysfs_show(struct device *dev,
| > +				struct device_attribute *attr, char *page);
| > +
| > +#define EVENT_VAR(_id)	event_attr_##_id
| > +#define EVENT_PTR(_id) &event_attr_##_id.attr.attr
| > +
| > +#define EVENT_ATTR(_name, _id)                                             \
| > +	static struct perf_pmu_events_attr EVENT_VAR(_id) = {              \
| > +		.attr = __ATTR(_name, 0444, power_events_sysfs_show, NULL),\
| > +		.id   = PM_##_id,                                               \
| > +	};
| 
| this is duplicating the x86 code, perhaps it could be moved
| to include/linux/perf_event.h and shared globaly

Ok.

Can we remove the assumption that the event id is a generic event that
has PERF_COUNT_HW_ prefix and also let the architectures pass in a "show"
function ? This would allow architectures to display any arch specific
events that don't yet have a generic counterpart.

IOW, can we do something like this (untested) and make PERF_EVENT_ATTR global:




| 
| 
| > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
| > index aa2465e..19b23bd 100644
| > --- a/arch/powerpc/perf/core-book3s.c
| > +++ b/arch/powerpc/perf/core-book3s.c
| > @@ -1305,6 +1305,16 @@ static int power_pmu_event_idx(struct perf_event *event)
| >  	return event->hw.idx;
| >  }
| >  
| > +ssize_t power_events_sysfs_show(struct device *dev,
| > +				struct device_attribute *attr, char *page)
| > +{
| > +       struct perf_pmu_events_attr *pmu_attr;
| > +       
| > +       pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
| > +        
| > +       return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
| 
| whitespace issues

Will fix. Thanks for the review.

Sukadev

On Wed, Nov 14, 2012 at 10:20:45AM -0800, Sukadev Bhattiprolu wrote:
> Jiri Olsa [jolsa@redhat.com] wrote:
> | On Wed, Nov 07, 2012 at 11:19:28AM -0800, Sukadev Bhattiprolu wrote:
> | 
> | SNIP
> | 
> | > +struct perf_pmu_events_attr {
> | > +	struct device_attribute attr;
> | > +	u64 id;
> | > +};
> | > +
> | > +extern ssize_t power_events_sysfs_show(struct device *dev,
> | > +				struct device_attribute *attr, char *page);
> | > +
> | > +#define EVENT_VAR(_id)	event_attr_##_id
> | > +#define EVENT_PTR(_id) &event_attr_##_id.attr.attr
> | > +
> | > +#define EVENT_ATTR(_name, _id)                                             \
> | > +	static struct perf_pmu_events_attr EVENT_VAR(_id) = {              \
> | > +		.attr = __ATTR(_name, 0444, power_events_sysfs_show, NULL),\
> | > +		.id   = PM_##_id,                                               \
> | > +	};
> | 
> | this is duplicating the x86 code, perhaps it could be moved
> | to include/linux/perf_event.h and shared globaly
> 
> Ok.
> 
> Can we remove the assumption that the event id is a generic event that
> has PERF_COUNT_HW_ prefix and also let the architectures pass in a "show"
> function ? This would allow architectures to display any arch specific
> events that don't yet have a generic counterpart.
> 
> IOW, can we do something like this (untested) and make PERF_EVENT_ATTR global:

hm, then you probably can use following:

http://www.spinics.net/lists/kernel/msg1434233.html
http://www.spinics.net/lists/kernel/msg1434235.html
http://www.spinics.net/lists/kernel/msg1434226.html

jirka

Jiri Olsa [jolsa@redhat.com] wrote:
| > 
| > Can we remove the assumption that the event id is a generic event that
| > has PERF_COUNT_HW_ prefix and also let the architectures pass in a "show"
| > function ? This would allow architectures to display any arch specific
| > events that don't yet have a generic counterpart.
| > 
| > IOW, can we do something like this (untested) and make PERF_EVENT_ATTR global:
| 
| hm, then you probably can use following:
| 
| http://www.spinics.net/lists/kernel/msg1434233.html

For now, power events can simply be u64 - so am hoping to have something
like this and replace the raw codes:

#define PM_CYC			0x1e
#define PM_GCT_NOSLOT_CYC	0x100f8

EVENT_ATTR_STR() is interesting, but would require new/extra macros like

#define PM_CYC_STR		"0x1e"
#define PM_GCT_NOSLOT_CYC_STR	"0x100f8"

I went down the path we discussed earlier - to have x86 and Power share
the EVENT_ATTR() macros. This is making the x86 code less concise

	EVENT_ATTR(cpu-cycles,	CPU_CYCLES)

becomes

	EVENT_ATTR(cpu-cycles,	PERF_COUNT_HW_CPU_CYCLES, events_sysfs_show)

with each EVENT_ATTR() explcitly adding events_sysfs_show or needing another
wrapper around the wrappers.

Still tweaking it, but would it be flexible to make 'struct pmu_events_attr'
common and let architectures define the EVENT_ATTR() as needed ? Would
duplicate some code but hopefully not a lot.

Sukadev

On Fri, Nov 16, 2012 at 11:35:37AM -0800, Sukadev Bhattiprolu wrote:
> Jiri Olsa [jolsa@redhat.com] wrote:
> | > 
> | > Can we remove the assumption that the event id is a generic event that
> | > has PERF_COUNT_HW_ prefix and also let the architectures pass in a "show"
> | > function ? This would allow architectures to display any arch specific
> | > events that don't yet have a generic counterpart.
> | > 
> | > IOW, can we do something like this (untested) and make PERF_EVENT_ATTR global:
> | 
> | hm, then you probably can use following:
> | 
> | http://www.spinics.net/lists/kernel/msg1434233.html
> 
> For now, power events can simply be u64 - so am hoping to have something
> like this and replace the raw codes:
> 
> #define PM_CYC			0x1e
> #define PM_GCT_NOSLOT_CYC	0x100f8
> 
> EVENT_ATTR_STR() is interesting, but would require new/extra macros like
> 
> #define PM_CYC_STR		"0x1e"
> #define PM_GCT_NOSLOT_CYC_STR	"0x100f8"
> 
> I went down the path we discussed earlier - to have x86 and Power share
> the EVENT_ATTR() macros. This is making the x86 code less concise
> 
> 	EVENT_ATTR(cpu-cycles,	CPU_CYCLES)
> 
> becomes
> 
> 	EVENT_ATTR(cpu-cycles,	PERF_COUNT_HW_CPU_CYCLES, events_sysfs_show)
> 
> with each EVENT_ATTR() explcitly adding events_sysfs_show or needing another
> wrapper around the wrappers.
> 
> Still tweaking it, but would it be flexible to make 'struct pmu_events_attr'
> common and let architectures define the EVENT_ATTR() as needed ? Would
> duplicate some code but hopefully not a lot.

sounds good, let's see ;)

jirka