Patchwork [2/5] perf: Make EVENT_ATTR and EVENT_PTR global

login
register
mail settings
Submitter sukadev@linux.vnet.ibm.com
Date Dec. 19, 2012, 7:28 a.m.
Message ID <20121219072802.GA30790@us.ibm.com>
Download mbox | patch
Permalink /patch/207252/
State Superseded
Headers show

Comments

sukadev@linux.vnet.ibm.com - Dec. 19, 2012, 7:28 a.m.
Rename EVENT_ATTR() and EVENT_PTR() PMU_EVENT_ATTR() and PMU_EVENT_PTR().
Make them global so they are available to all architectures.

Further to allow architectures flexibility, have PMU_EVENT_PTR() pass in the
variable name as a parameter.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/x86/kernel/cpu/perf_event.c |   17 +++++------------
 include/linux/perf_event.h       |   13 +++++++++++++
 2 files changed, 18 insertions(+), 12 deletions(-)
Jiri Olsa - Jan. 2, 2013, 2:58 p.m.
On Tue, Dec 18, 2012 at 11:28:02PM -0800, Sukadev Bhattiprolu wrote:
> 
> Rename EVENT_ATTR() and EVENT_PTR() PMU_EVENT_ATTR() and PMU_EVENT_PTR().
> Make them global so they are available to all architectures.
> 
> Further to allow architectures flexibility, have PMU_EVENT_PTR() pass in the
> variable name as a parameter.
> 
hi,
the change looks ok apart from some nits below.

There' another version of the x86 event attributes change
I mentioned earlier:

http://marc.info/?l=linux-kernel&m=135601815224373&w=2

I'm not sure which one will make it in first, but you
guys need to sync ;-) CC-ing Andi and Stephane.

thanks,
jirka

> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
>  arch/x86/kernel/cpu/perf_event.c |   17 +++++------------
>  include/linux/perf_event.h       |   13 +++++++++++++
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 4428fd1..24bc505 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1316,11 +1316,6 @@ static struct attribute_group x86_pmu_format_group = {
>  	.attrs = NULL,
>  };
>  
> -struct perf_pmu_events_attr {
> -	struct device_attribute attr;
> -	u64 id;
> -};
> -
>  /*
>   * Remove all undefined events (x86_pmu.event_map(id) == 0)
>   * out of events_attr attributes.
> @@ -1351,14 +1346,12 @@ static ssize_t events_sysfs_show(struct device *dev, struct device_attribute *at
>  	return x86_pmu.events_sysfs_show(page, config);
>  }
>  
> -#define EVENT_VAR(_id)  event_attr_##_id
> -#define EVENT_PTR(_id) &event_attr_##_id.attr.attr
> +#define EVENT_VAR(_id)	event_attr_##_id
> +#define EVENT_ID(_id)	PERF_COUNT_HW_##_id
> +#define EVENT_PTR(_id)	PMU_EVENT_PTR(EVENT_VAR(_id))
>  
> -#define EVENT_ATTR(_name, _id)					\
> -static struct perf_pmu_events_attr EVENT_VAR(_id) = {		\
> -	.attr = __ATTR(_name, 0444, events_sysfs_show, NULL),	\
> -	.id   =  PERF_COUNT_HW_##_id,				\
> -};
> +#define EVENT_ATTR(_name, _id)						\
> +	PMU_EVENT_ATTR(_name, EVENT_VAR(_id), EVENT_ID(_id), events_sysfs_show)

probably no need to define EVENT_ID macro if it's used on just one place

>  
>  EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
>  EVENT_ATTR(instructions,		INSTRUCTIONS		);
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 6bfb2fa..31692cb 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -817,6 +817,19 @@ do {									\
>  } while (0)
>  
>  
> +struct perf_pmu_events_attr {
> +	struct device_attribute attr;
> +	u64 id;
> +};
> +
> +#define PMU_EVENT_PTR(_var)	&_var.attr.attr

this one seems superfluous as well, could be replaced by '&'
sukadev@linux.vnet.ibm.com - Jan. 5, 2013, 1:47 a.m.
Jiri Olsa [jolsa@redhat.com] wrote:
| On Tue, Dec 18, 2012 at 11:28:02PM -0800, Sukadev Bhattiprolu wrote:
| > 
| > Rename EVENT_ATTR() and EVENT_PTR() PMU_EVENT_ATTR() and PMU_EVENT_PTR().
| > Make them global so they are available to all architectures.
| > 
| > Further to allow architectures flexibility, have PMU_EVENT_PTR() pass in the
| > variable name as a parameter.
| > 
| hi,
| the change looks ok apart from some nits below.
| 
| There' another version of the x86 event attributes change
| I mentioned earlier:
| 
| http://marc.info/?l=linux-kernel&m=135601815224373&w=2
| 
| I'm not sure which one will make it in first, but you
| guys need to sync ;-) CC-ing Andi and Stephane.

One change that would help powerpc (and other architectures) is to move
the 'struct perf_pmu_events_attr' to say, include/linux/perf_event.h.

Each architecture can define EVENT_VAR(), EVENT_PTR() etc as needed.

| 
| thanks,
| jirka
| 

<snip>

| > +struct perf_pmu_events_attr {
| > +	struct device_attribute attr;
| > +	u64 id;
| > +};
| > +
| > +#define PMU_EVENT_PTR(_var)	&_var.attr.attr
| 
| this one seems superfluous as well, could be replaced by '&'

I guess that would encode the assumption that both the 'attr' fields are 
the first in their respective structures. If so, an explicit comment beside
the fields would be useful.

Sukadev

Patch

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 4428fd1..24bc505 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1316,11 +1316,6 @@  static struct attribute_group x86_pmu_format_group = {
 	.attrs = NULL,
 };
 
-struct perf_pmu_events_attr {
-	struct device_attribute attr;
-	u64 id;
-};
-
 /*
  * Remove all undefined events (x86_pmu.event_map(id) == 0)
  * out of events_attr attributes.
@@ -1351,14 +1346,12 @@  static ssize_t events_sysfs_show(struct device *dev, struct device_attribute *at
 	return x86_pmu.events_sysfs_show(page, config);
 }
 
-#define EVENT_VAR(_id)  event_attr_##_id
-#define EVENT_PTR(_id) &event_attr_##_id.attr.attr
+#define EVENT_VAR(_id)	event_attr_##_id
+#define EVENT_ID(_id)	PERF_COUNT_HW_##_id
+#define EVENT_PTR(_id)	PMU_EVENT_PTR(EVENT_VAR(_id))
 
-#define EVENT_ATTR(_name, _id)					\
-static struct perf_pmu_events_attr EVENT_VAR(_id) = {		\
-	.attr = __ATTR(_name, 0444, events_sysfs_show, NULL),	\
-	.id   =  PERF_COUNT_HW_##_id,				\
-};
+#define EVENT_ATTR(_name, _id)						\
+	PMU_EVENT_ATTR(_name, EVENT_VAR(_id), EVENT_ID(_id), events_sysfs_show)
 
 EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
 EVENT_ATTR(instructions,		INSTRUCTIONS		);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 6bfb2fa..31692cb 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -817,6 +817,19 @@  do {									\
 } while (0)
 
 
+struct perf_pmu_events_attr {
+	struct device_attribute attr;
+	u64 id;
+};
+
+#define PMU_EVENT_PTR(_var)	&_var.attr.attr
+
+#define PMU_EVENT_ATTR(_name, _var, _id, _show)				\
+static struct perf_pmu_events_attr _var = {				\
+	.attr = __ATTR(_name, 0444, _show, NULL),			\
+	.id   =  _id,							\
+};
+
 #define PMU_FORMAT_ATTR(_name, _format)					\
 static ssize_t								\
 _name##_show(struct device *dev,					\