| Message ID | 1572407177-48229-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: perf: Simplify the ARMv8 PMUv3 event attributes | expand |
On 10/30/19 4:46 AM, Shaokun Zhang wrote: > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > to simplify the armv8_pmuv3_event_attrs. ... > #define ARMV8_EVENT_ATTR(name, config) \ > + (&((struct perf_pmu_events_attr[]) { \ > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > + .id = config, } \ > + })[0].attr.attr) > > static struct attribute *armv8_pmuv3_event_attrs[] = { > + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), You do realize this creates complete perf_pmu_events_attr structures, most of which is unused and unreachable, right? Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs array cannot get out of sync with the ARMV8_PMUV3_* defines? Slightly better would seem to be #define ARMV8_EVENT_ATTR(name, config) \ [config] = &((struct device_attribute) \ __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr though I'm not sure why __ATTR is particularly desired above #define ARMV8_EVENT_ATTR(name, config) \ [config] = &(struct attribute){ \ .name = __stringify(name), \ .mode = 0444, \ } r~
On Wed, Oct 30, 2019 at 02:34:37PM +0100, Richard Henderson wrote: > On 10/30/19 4:46 AM, Shaokun Zhang wrote: > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > to simplify the armv8_pmuv3_event_attrs. > ... > > #define ARMV8_EVENT_ATTR(name, config) \ > > + (&((struct perf_pmu_events_attr[]) { \ > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > + .id = config, } \ > > + })[0].attr.attr) > > > > static struct attribute *armv8_pmuv3_event_attrs[] = { > > + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), > > You do realize this creates complete perf_pmu_events_attr structures, most of > which is unused and unreachable, right? In armv8pmu_events_sysfs_show() we use container_of() to access the perf_pmu_events_attr, and extracts the id field: | static ssize_t | armv8pmu_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%03llx\n", pmu_attr->id); | } > Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs > array cannot get out of sync with the ARMV8_PMUV3_* defines? > > Slightly better would seem to be > > #define ARMV8_EVENT_ATTR(name, config) \ > [config] = &((struct device_attribute) \ > __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr I'm not sure I follow. This is not equivalent, and you're using the config field in a very different way -- that's not an index in the parent array in the current code. How do you expect armv8pmu_events_sysfs_show to get the config value in this case? > > though I'm not sure why __ATTR is particularly desired above > > #define ARMV8_EVENT_ATTR(name, config) \ > [config] = &(struct attribute){ \ > .name = __stringify(name), \ > .mode = 0444, \ > } Using __ATTR is consistent with other drivers, so I don't see a reason to change that unless there's a significant simplification, or a functional improvement Thanks, Mark.
Hi Richard, Thanks your comments and Mark has helped to reply some. On 2019/10/30 21:34, Richard Henderson wrote: > On 10/30/19 4:46 AM, Shaokun Zhang wrote: >> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >> to simplify the armv8_pmuv3_event_attrs. > ... >> #define ARMV8_EVENT_ATTR(name, config) \ >> + (&((struct perf_pmu_events_attr[]) { \ >> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >> + .id = config, } \ >> + })[0].attr.attr) >> >> static struct attribute *armv8_pmuv3_event_attrs[] = { >> + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), > > You do realize this creates complete perf_pmu_events_attr structures, most of > which is unused and unreachable, right? > > Also, why not take the opportunity to assert that the armv8_pmuv3_event_attrs > array cannot get out of sync with the ARMV8_PMUV3_* defines? > For my initial purpose: remove the &armv8_event_attr_xx.attr.attr and only maintain the armv8_pmuv3_event_attrs array directly when we want to add one new PMU event. For example: #define ARMV8_PMUV3_PERFCTR_OP_RETIRED 0x3A ..... static struct attribute *armv8_pmuv3_event_attrs[] = { ...... ARMV8_EVENT_ATTR(op_retired, ARMV8_PMUV3_PERFCTR_OP_RETIRED), NULL, }; Thanks, Shaokun > Slightly better would seem to be > > #define ARMV8_EVENT_ATTR(name, config) \ > [config] = &((struct device_attribute) \ > __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL)).attr > > though I'm not sure why __ATTR is particularly desired above > > #define ARMV8_EVENT_ATTR(name, config) \ > [config] = &(struct attribute){ \ > .name = __stringify(name), \ > .mode = 0444, \ > } > > > r~ > >
On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > to simplify the armv8_pmuv3_event_attrs. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > --- > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > 1 file changed, 65 insertions(+), 124 deletions(-) > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index a0b4f1bca491..d0f084939bcf 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > } [...] > + (&((struct perf_pmu_events_attr[]) { \ > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > + .id = config, } \ > + })[0].attr.attr) I don't get the need for the array here. Why can't you do: (&((struct perf_pmu_events_attr) { .attr = ..., .id = ..., }).attr.attr) ? Will
Hi Will, On 2019/11/1 0:08, Will Deacon wrote: > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >> to simplify the armv8_pmuv3_event_attrs. >> >> Cc: Will Deacon <will@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >> --- >> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >> 1 file changed, 65 insertions(+), 124 deletions(-) >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index a0b4f1bca491..d0f084939bcf 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >> } > > [...] > >> + (&((struct perf_pmu_events_attr[]) { \ >> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >> + .id = config, } \ >> + })[0].attr.attr) > > I don't get the need for the array here. Why can't you do: > > (&((struct perf_pmu_events_attr) { > .attr = ..., > .id = ..., > }).attr.attr) > > ? I try it and there is a compiler error: zhangshaokun@ubuntu:~/linux$ make Image -j64 CALL scripts/atomic/check-atomics.sh CALL scripts/checksyscalls.sh CHK include/generated/compile.h CC arch/arm64/kernel/perf_event.o arch/arm64/kernel/perf_event.c:162:13: error: unknown field ‘attr’ specified in initializer (&((struct perf_pmu_events_attr) { \ ^ arch/arm64/kernel/perf_event.c:168:2: note: in expansion of macro ‘ARMV8_EVENT_ATTR’ ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), ^ arch/arm64/kernel/perf_event.c:162:13: warning: braces around scalar initializer (&((struct perf_pmu_events_attr) { \ ^ arch/arm64/kernel/perf_event.c:168:2: note: in expansion of macro ‘ARMV8_EVENT_ATTR’ ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), It seems that it became completely anonymous and the compiler can't find its member. Thanks, Shaokun > > Will > > . >
On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > to simplify the armv8_pmuv3_event_attrs. > > > > Cc: Will Deacon <will@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > --- > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > > 1 file changed, 65 insertions(+), 124 deletions(-) > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > index a0b4f1bca491..d0f084939bcf 100644 > > --- a/arch/arm64/kernel/perf_event.c > > +++ b/arch/arm64/kernel/perf_event.c > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > > } > > [...] > > > + (&((struct perf_pmu_events_attr[]) { \ > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > + .id = config, } \ > > + })[0].attr.attr) > > I don't get the need for the array here. Why can't you do: > > (&((struct perf_pmu_events_attr) { > .attr = ..., > .id = ..., > }).attr.attr) You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. #define ARMV8_EVENT_ATTR(name, config) \ (&((struct perf_pmu_events_attr) { \ .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ .id = config, \ }.attr.attr)) ... which compiles for me. It'd be worth checking that yields a working data structure at runtime. I'm not sure why I did the array hack in the other PMU drivers -- looks like we can simplify those assuming this works. :) THanks, Mark.
On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > > to simplify the armv8_pmuv3_event_attrs. > > > > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > --- > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > > > 1 file changed, 65 insertions(+), 124 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > > index a0b4f1bca491..d0f084939bcf 100644 > > > --- a/arch/arm64/kernel/perf_event.c > > > +++ b/arch/arm64/kernel/perf_event.c > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > > > } > > > > [...] > > > > > + (&((struct perf_pmu_events_attr[]) { \ > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > + .id = config, } \ > > > + })[0].attr.attr) > > > > I don't get the need for the array here. Why can't you do: > > > > (&((struct perf_pmu_events_attr) { > > .attr = ..., > > .id = ..., > > }).attr.attr) > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. > > #define ARMV8_EVENT_ATTR(name, config) \ > (&((struct perf_pmu_events_attr) { \ > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > .id = config, \ > }.attr.attr)) > > ... which compiles for me. Weird, the following compiles fine for me with both GCC and clang: #define ARMV8_EVENT_ATTR(name, config) \ (&((struct perf_pmu_events_attr) { \ .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ .id = config, \ }).attr.attr) > It'd be worth checking that yields a working data structure at runtime. ...and perf list works as expected. Will
On 2019-11-01 10:36 am, Will Deacon wrote: > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>> to simplify the armv8_pmuv3_event_attrs. >>>> >>>> Cc: Will Deacon <will@kernel.org> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>> --- >>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>> index a0b4f1bca491..d0f084939bcf 100644 >>>> --- a/arch/arm64/kernel/perf_event.c >>>> +++ b/arch/arm64/kernel/perf_event.c >>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>> } >>> >>> [...] >>> >>>> + (&((struct perf_pmu_events_attr[]) { \ >>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>> + .id = config, } \ >>>> + })[0].attr.attr) >>> >>> I don't get the need for the array here. Why can't you do: >>> >>> (&((struct perf_pmu_events_attr) { >>> .attr = ..., >>> .id = ..., >>> }).attr.attr) >> >> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >> >> #define ARMV8_EVENT_ATTR(name, config) \ >> (&((struct perf_pmu_events_attr) { \ >> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >> .id = config, \ >> }.attr.attr)) >> >> ... which compiles for me. > > Weird, the following compiles fine for me with both GCC and clang: > > #define ARMV8_EVENT_ATTR(name, config) \ > (&((struct perf_pmu_events_attr) { \ > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > .id = config, \ > }).attr.attr) You know that the expressions are equivalent because unary "&" has lower precedence than ".", right? ;) Robin. >> It'd be worth checking that yields a working data structure at runtime. > > ...and perf list works as expected. > > Will > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: > On 2019-11-01 10:36 am, Will Deacon wrote: > > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: > > > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: > > > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > > > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > > > > to simplify the armv8_pmuv3_event_attrs. > > > > > > > > > > Cc: Will Deacon <will@kernel.org> > > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > > > --- > > > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > > > > > 1 file changed, 65 insertions(+), 124 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > > > > index a0b4f1bca491..d0f084939bcf 100644 > > > > > --- a/arch/arm64/kernel/perf_event.c > > > > > +++ b/arch/arm64/kernel/perf_event.c > > > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > > > > > } > > > > > > > > [...] > > > > > > > > > + (&((struct perf_pmu_events_attr[]) { \ > > > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > > + .id = config, } \ > > > > > + })[0].attr.attr) > > > > > > > > I don't get the need for the array here. Why can't you do: > > > > > > > > (&((struct perf_pmu_events_attr) { > > > > .attr = ..., > > > > .id = ..., > > > > }).attr.attr) > > > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. > > > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > > (&((struct perf_pmu_events_attr) { \ > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > .id = config, \ > > > }.attr.attr)) > > > ... which compiles for me. > > > > Weird, the following compiles fine for me with both GCC and clang: > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > (&((struct perf_pmu_events_attr) { \ > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > .id = config, \ > > }).attr.attr) > > You know that the expressions are equivalent because unary "&" has lower > precedence than ".", right? ;) Right, which is why it's weird that Shaokun claims that the version I posted doesn't compile. I assume it didn't build for Mark either, hence his extra brackets. Will
On 2019-11-01 10:55 am, Will Deacon wrote: > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: >> On 2019-11-01 10:36 am, Will Deacon wrote: >>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>>>> to simplify the armv8_pmuv3_event_attrs. >>>>>> >>>>>> Cc: Will Deacon <will@kernel.org> >>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>>>> --- >>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>>>> index a0b4f1bca491..d0f084939bcf 100644 >>>>>> --- a/arch/arm64/kernel/perf_event.c >>>>>> +++ b/arch/arm64/kernel/perf_event.c >>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>>>> } >>>>> >>>>> [...] >>>>> >>>>>> + (&((struct perf_pmu_events_attr[]) { \ >>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>> + .id = config, } \ >>>>>> + })[0].attr.attr) >>>>> >>>>> I don't get the need for the array here. Why can't you do: >>>>> >>>>> (&((struct perf_pmu_events_attr) { >>>>> .attr = ..., >>>>> .id = ..., >>>>> }).attr.attr) >>>> >>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >>>> >>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>> (&((struct perf_pmu_events_attr) { \ >>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>> .id = config, \ >>>> }.attr.attr)) >>>> ... which compiles for me. >>> >>> Weird, the following compiles fine for me with both GCC and clang: >>> >>> #define ARMV8_EVENT_ATTR(name, config) \ >>> (&((struct perf_pmu_events_attr) { \ >>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>> .id = config, \ >>> }).attr.attr) >> >> You know that the expressions are equivalent because unary "&" has lower >> precedence than ".", right? ;) > > Right, which is why it's weird that Shaokun claims that the version I posted > doesn't compile. I assume it didn't build for Mark either, hence his extra > brackets. Because different compilers have different ideas of whether "obj" is a valid thing to dereference at all, regardless of where you put parentheses. From what I remember, the array trick was the only way to convince older GCCs to treat the floating struct initialiser as an actual object definition. I guess newer versions are a bit more lenient. Robin.
On Fri, Nov 01, 2019 at 10:36:17AM +0000, Will Deacon wrote: > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: > > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: > > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > > > to simplify the armv8_pmuv3_event_attrs. > > > > > > > > Cc: Will Deacon <will@kernel.org> > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > > --- > > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > > > > 1 file changed, 65 insertions(+), 124 deletions(-) > > > > > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > > > index a0b4f1bca491..d0f084939bcf 100644 > > > > --- a/arch/arm64/kernel/perf_event.c > > > > +++ b/arch/arm64/kernel/perf_event.c > > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > > > > } > > > > > > [...] > > > > > > > + (&((struct perf_pmu_events_attr[]) { \ > > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > + .id = config, } \ > > > > + })[0].attr.attr) > > > > > > I don't get the need for the array here. Why can't you do: > > > > > > (&((struct perf_pmu_events_attr) { > > > .attr = ..., > > > .id = ..., > > > }).attr.attr) > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > (&((struct perf_pmu_events_attr) { \ > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > .id = config, \ > > }.attr.attr)) > > > > ... which compiles for me. > > Weird, the following compiles fine for me with both GCC and clang: > > #define ARMV8_EVENT_ATTR(name, config) \ > (&((struct perf_pmu_events_attr) { \ > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > .id = config, \ > }).attr.attr) Works for me, too (I'm using the kernel.org crosstool GCC 8.1.0). I must've messed up locally such that I had (&obj).attr.attr. > > It'd be worth checking that yields a working data structure at runtime. > > ...and perf list works as expected. Perfect. Thanks, Mark.
On Fri, Nov 01, 2019 at 11:11:49AM +0000, Robin Murphy wrote: > On 2019-11-01 10:55 am, Will Deacon wrote: > > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: > > > On 2019-11-01 10:36 am, Will Deacon wrote: > > > > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: > > > > > On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: > > > > > > On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: > > > > > > > For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and > > > > > > > &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR > > > > > > > to simplify the armv8_pmuv3_event_attrs. > > > > > > > > > > > > > > Cc: Will Deacon <will@kernel.org> > > > > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > > > > > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> > > > > > > > --- > > > > > > > arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- > > > > > > > 1 file changed, 65 insertions(+), 124 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > > > > > > index a0b4f1bca491..d0f084939bcf 100644 > > > > > > > --- a/arch/arm64/kernel/perf_event.c > > > > > > > +++ b/arch/arm64/kernel/perf_event.c > > > > > > > @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, > > > > > > > } > > > > > > > > > > > > [...] > > > > > > > > > > > > > + (&((struct perf_pmu_events_attr[]) { \ > > > > > > > + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > > > > + .id = config, } \ > > > > > > > + })[0].attr.attr) > > > > > > > > > > > > I don't get the need for the array here. Why can't you do: > > > > > > > > > > > > (&((struct perf_pmu_events_attr) { > > > > > > .attr = ..., > > > > > > .id = ..., > > > > > > }).attr.attr) > > > > > > > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. > > > > > > > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > > > > (&((struct perf_pmu_events_attr) { \ > > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > > .id = config, \ > > > > > }.attr.attr)) > > > > > ... which compiles for me. > > > > > > > > Weird, the following compiles fine for me with both GCC and clang: > > > > > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > > > (&((struct perf_pmu_events_attr) { \ > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > .id = config, \ > > > > }).attr.attr) > > > > > > You know that the expressions are equivalent because unary "&" has lower > > > precedence than ".", right? ;) > > > > Right, which is why it's weird that Shaokun claims that the version I posted > > doesn't compile. I assume it didn't build for Mark either, hence his extra > > brackets. I must've meessed up locally -- sorry for the noise. > Because different compilers have different ideas of whether "obj" is a valid > thing to dereference at all, regardless of where you put parentheses. From > what I remember, the array trick was the only way to convince older GCCs to > treat the floating struct initialiser as an actual object definition. I > guess newer versions are a bit more lenient. I strongly suspect Will's (much cleaner) version would work with those older compilers too, and I just didn't know what I was doing ~8 years ago when I came up with the trick. I can have a go with my toolchain museum on Monday; if old GCCs are happy we can clean up the other instances of the trick to be much more legible. Thanks, Mark.
On Fri, Nov 01, 2019 at 02:36:03PM +0000, Mark Rutland wrote: > On Fri, Nov 01, 2019 at 11:11:49AM +0000, Robin Murphy wrote: > > On 2019-11-01 10:55 am, Will Deacon wrote: > > > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: > > > > On 2019-11-01 10:36 am, Will Deacon wrote: > > > > > On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: > > > > > > You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. > > > > > > > > > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > > > > > (&((struct perf_pmu_events_attr) { \ > > > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > > > .id = config, \ > > > > > > }.attr.attr)) > > > > > > ... which compiles for me. > > > > > > > > > > Weird, the following compiles fine for me with both GCC and clang: > > > > > > > > > > #define ARMV8_EVENT_ATTR(name, config) \ > > > > > (&((struct perf_pmu_events_attr) { \ > > > > > .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ > > > > > .id = config, \ > > > > > }).attr.attr) > > > > > > > > You know that the expressions are equivalent because unary "&" has lower > > > > precedence than ".", right? ;) > > > > > > Right, which is why it's weird that Shaokun claims that the version I posted > > > doesn't compile. I assume it didn't build for Mark either, hence his extra > > > brackets. > > I must've meessed up locally -- sorry for the noise. > > > Because different compilers have different ideas of whether "obj" is a valid > > thing to dereference at all, regardless of where you put parentheses. From > > what I remember, the array trick was the only way to convince older GCCs to > > treat the floating struct initialiser as an actual object definition. I > > guess newer versions are a bit more lenient. > > I strongly suspect Will's (much cleaner) version would work with those older > compilers too, and I just didn't know what I was doing ~8 years ago when I came > up with the trick. > > I can have a go with my toolchain museum on Monday; if old GCCs are happy we > can clean up the other instances of the trick to be much more legible. I've thrown it into -next to see how it gets on. Will
Hi Will, On 2019/11/1 18:55, Will Deacon wrote: > On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: >> On 2019-11-01 10:36 am, Will Deacon wrote: >>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>>>> to simplify the armv8_pmuv3_event_attrs. >>>>>> >>>>>> Cc: Will Deacon <will@kernel.org> >>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>>>> --- >>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>>>> index a0b4f1bca491..d0f084939bcf 100644 >>>>>> --- a/arch/arm64/kernel/perf_event.c >>>>>> +++ b/arch/arm64/kernel/perf_event.c >>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>>>> } >>>>> >>>>> [...] >>>>> >>>>>> + (&((struct perf_pmu_events_attr[]) { \ >>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>> + .id = config, } \ >>>>>> + })[0].attr.attr) >>>>> >>>>> I don't get the need for the array here. Why can't you do: >>>>> >>>>> (&((struct perf_pmu_events_attr) { >>>>> .attr = ..., >>>>> .id = ..., >>>>> }).attr.attr) >>>> >>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >>>> >>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>> (&((struct perf_pmu_events_attr) { \ >>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>> .id = config, \ >>>> }.attr.attr)) >>>> ... which compiles for me. >>> >>> Weird, the following compiles fine for me with both GCC and clang: >>> >>> #define ARMV8_EVENT_ATTR(name, config) \ >>> (&((struct perf_pmu_events_attr) { \ >>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>> .id = config, \ >>> }).attr.attr) >> >> You know that the expressions are equivalent because unary "&" has lower >> precedence than ".", right? ;) > > Right, which is why it's weird that Shaokun claims that the version I posted > doesn't compile. I assume it didn't build for Mark either, hence his extra The GCC version is gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609. It's a little old ;-) Thanks, Shaokun > brackets. > > Will > > . >
Hi Robin, On 2019/11/1 19:11, Robin Murphy wrote: > On 2019-11-01 10:55 am, Will Deacon wrote: >> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: >>> On 2019-11-01 10:36 am, Will Deacon wrote: >>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>>>>> to simplify the armv8_pmuv3_event_attrs. >>>>>>> >>>>>>> Cc: Will Deacon <will@kernel.org> >>>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>>>>> --- >>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>>>>> index a0b4f1bca491..d0f084939bcf 100644 >>>>>>> --- a/arch/arm64/kernel/perf_event.c >>>>>>> +++ b/arch/arm64/kernel/perf_event.c >>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>>>>> } >>>>>> >>>>>> [...] >>>>>> >>>>>>> + (&((struct perf_pmu_events_attr[]) { \ >>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>>> + .id = config, } \ >>>>>>> + })[0].attr.attr) >>>>>> >>>>>> I don't get the need for the array here. Why can't you do: >>>>>> >>>>>> (&((struct perf_pmu_events_attr) { >>>>>> .attr = ..., >>>>>> .id = ..., >>>>>> }).attr.attr) >>>>> >>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >>>>> >>>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>>> (&((struct perf_pmu_events_attr) { \ >>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>> .id = config, \ >>>>> }.attr.attr)) >>>>> ... which compiles for me. >>>> >>>> Weird, the following compiles fine for me with both GCC and clang: >>>> >>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>> (&((struct perf_pmu_events_attr) { \ >>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>> .id = config, \ >>>> }).attr.attr) >>> >>> You know that the expressions are equivalent because unary "&" has lower >>> precedence than ".", right? ;) >> >> Right, which is why it's weird that Shaokun claims that the version I posted >> doesn't compile. I assume it didn't build for Mark either, hence his extra >> brackets. > > Because different compilers have different ideas of whether "obj" is a valid thing to dereference at all, regardless of where you put parentheses. From what I remember, the array trick was the only way to convince older GCCs to treat the floating struct initialiser as an actual object definition. I guess newer versions are a bit more lenient. > Thanks for your detailed explanations, sounds great! Both GCC 5.4 and 7.3 are unhappy without the array trick. Thanks, Shaokun > Robin. > > . >
Hi Will, On 2019/11/4 9:02, Shaokun Zhang wrote: > Hi Will, > > On 2019/11/1 18:55, Will Deacon wrote: >> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: >>> On 2019-11-01 10:36 am, Will Deacon wrote: >>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>>>>> to simplify the armv8_pmuv3_event_attrs. >>>>>>> >>>>>>> Cc: Will Deacon <will@kernel.org> >>>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>>>>> --- >>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>>>>> index a0b4f1bca491..d0f084939bcf 100644 >>>>>>> --- a/arch/arm64/kernel/perf_event.c >>>>>>> +++ b/arch/arm64/kernel/perf_event.c >>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>>>>> } >>>>>> >>>>>> [...] >>>>>> >>>>>>> + (&((struct perf_pmu_events_attr[]) { \ >>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>>> + .id = config, } \ >>>>>>> + })[0].attr.attr) >>>>>> >>>>>> I don't get the need for the array here. Why can't you do: >>>>>> >>>>>> (&((struct perf_pmu_events_attr) { >>>>>> .attr = ..., >>>>>> .id = ..., >>>>>> }).attr.attr) >>>>> >>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >>>>> >>>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>>> (&((struct perf_pmu_events_attr) { \ >>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>> .id = config, \ >>>>> }.attr.attr)) >>>>> ... which compiles for me. >>>> >>>> Weird, the following compiles fine for me with both GCC and clang: >>>> >>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>> (&((struct perf_pmu_events_attr) { \ >>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>> .id = config, \ >>>> }).attr.attr) >>> >>> You know that the expressions are equivalent because unary "&" has lower >>> precedence than ".", right? ;) >> >> Right, which is why it's weird that Shaokun claims that the version I posted >> doesn't compile. I assume it didn't build for Mark either, hence his extra > > The GCC version is gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609. > It's a little old ;-) Sorry for my noise, it's my mistake that I forgot to remove the brackets when deleted the array. #define ARMV8_EVENT_ATTR(name, config) \ - (&((struct perf_pmu_events_attr[]) { \ + (&((struct perf_pmu_events_attr) { \ { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ .id = config, } \ - })[0].attr.attr) + }).attr.attr) It works now if I follow your proposal completely.:-P Thanks, Shaokun > > Thanks, > Shaokun > >> brackets. >> >> Will >> >> . >>
Hi Robin, Sorry for the noise, it works on both GCC 5.4 and 7.3 using Will's method. Thanks, Shaokun On 2019/11/4 10:02, Shaokun Zhang wrote: > Hi Robin, > > On 2019/11/1 19:11, Robin Murphy wrote: >> On 2019-11-01 10:55 am, Will Deacon wrote: >>> On Fri, Nov 01, 2019 at 10:54:21AM +0000, Robin Murphy wrote: >>>> On 2019-11-01 10:36 am, Will Deacon wrote: >>>>> On Fri, Nov 01, 2019 at 08:53:19AM +0000, Mark Rutland wrote: >>>>>> On Thu, Oct 31, 2019 at 04:08:04PM +0000, Will Deacon wrote: >>>>>>> On Wed, Oct 30, 2019 at 11:46:17AM +0800, Shaokun Zhang wrote: >>>>>>>> For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and >>>>>>>> &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR >>>>>>>> to simplify the armv8_pmuv3_event_attrs. >>>>>>>> >>>>>>>> Cc: Will Deacon <will@kernel.org> >>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> >>>>>>>> --- >>>>>>>> arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- >>>>>>>> 1 file changed, 65 insertions(+), 124 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >>>>>>>> index a0b4f1bca491..d0f084939bcf 100644 >>>>>>>> --- a/arch/arm64/kernel/perf_event.c >>>>>>>> +++ b/arch/arm64/kernel/perf_event.c >>>>>>>> @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, >>>>>>>> } >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> + (&((struct perf_pmu_events_attr[]) { \ >>>>>>>> + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>>>> + .id = config, } \ >>>>>>>> + })[0].attr.attr) >>>>>>> >>>>>>> I don't get the need for the array here. Why can't you do: >>>>>>> >>>>>>> (&((struct perf_pmu_events_attr) { >>>>>>> .attr = ..., >>>>>>> .id = ..., >>>>>>> }).attr.attr) >>>>>> >>>>>> You need want &(obj.attr.attr) rather than &(obj).attr.attr, i.e. >>>>>> >>>>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>>>> (&((struct perf_pmu_events_attr) { \ >>>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>>> .id = config, \ >>>>>> }.attr.attr)) >>>>>> ... which compiles for me. >>>>> >>>>> Weird, the following compiles fine for me with both GCC and clang: >>>>> >>>>> #define ARMV8_EVENT_ATTR(name, config) \ >>>>> (&((struct perf_pmu_events_attr) { \ >>>>> .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ >>>>> .id = config, \ >>>>> }).attr.attr) >>>> >>>> You know that the expressions are equivalent because unary "&" has lower >>>> precedence than ".", right? ;) >>> >>> Right, which is why it's weird that Shaokun claims that the version I posted >>> doesn't compile. I assume it didn't build for Mark either, hence his extra >>> brackets. >> >> Because different compilers have different ideas of whether "obj" is a valid thing to dereference at all, regardless of where you put parentheses. From what I remember, the array trick was the only way to convince older GCCs to treat the floating struct initialiser as an actual object definition. I guess newer versions are a bit more lenient. >> > > Thanks for your detailed explanations, sounds great! Both GCC 5.4 and 7.3 are > unhappy without the array trick. > > Thanks, > Shaokun > >> Robin. >> >> . >>
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index a0b4f1bca491..d0f084939bcf 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -159,132 +159,73 @@ armv8pmu_events_sysfs_show(struct device *dev, } #define ARMV8_EVENT_ATTR(name, config) \ - PMU_EVENT_ATTR(name, armv8_event_attr_##name, \ - config, armv8pmu_events_sysfs_show) - -ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR); -ARMV8_EVENT_ATTR(l1i_cache_refill, ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL); -ARMV8_EVENT_ATTR(l1i_tlb_refill, ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL); -ARMV8_EVENT_ATTR(l1d_cache_refill, ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL); -ARMV8_EVENT_ATTR(l1d_cache, ARMV8_PMUV3_PERFCTR_L1D_CACHE); -ARMV8_EVENT_ATTR(l1d_tlb_refill, ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL); -ARMV8_EVENT_ATTR(ld_retired, ARMV8_PMUV3_PERFCTR_LD_RETIRED); -ARMV8_EVENT_ATTR(st_retired, ARMV8_PMUV3_PERFCTR_ST_RETIRED); -ARMV8_EVENT_ATTR(inst_retired, ARMV8_PMUV3_PERFCTR_INST_RETIRED); -ARMV8_EVENT_ATTR(exc_taken, ARMV8_PMUV3_PERFCTR_EXC_TAKEN); -ARMV8_EVENT_ATTR(exc_return, ARMV8_PMUV3_PERFCTR_EXC_RETURN); -ARMV8_EVENT_ATTR(cid_write_retired, ARMV8_PMUV3_PERFCTR_CID_WRITE_RETIRED); -ARMV8_EVENT_ATTR(pc_write_retired, ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED); -ARMV8_EVENT_ATTR(br_immed_retired, ARMV8_PMUV3_PERFCTR_BR_IMMED_RETIRED); -ARMV8_EVENT_ATTR(br_return_retired, ARMV8_PMUV3_PERFCTR_BR_RETURN_RETIRED); -ARMV8_EVENT_ATTR(unaligned_ldst_retired, ARMV8_PMUV3_PERFCTR_UNALIGNED_LDST_RETIRED); -ARMV8_EVENT_ATTR(br_mis_pred, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED); -ARMV8_EVENT_ATTR(cpu_cycles, ARMV8_PMUV3_PERFCTR_CPU_CYCLES); -ARMV8_EVENT_ATTR(br_pred, ARMV8_PMUV3_PERFCTR_BR_PRED); -ARMV8_EVENT_ATTR(mem_access, ARMV8_PMUV3_PERFCTR_MEM_ACCESS); -ARMV8_EVENT_ATTR(l1i_cache, ARMV8_PMUV3_PERFCTR_L1I_CACHE); -ARMV8_EVENT_ATTR(l1d_cache_wb, ARMV8_PMUV3_PERFCTR_L1D_CACHE_WB); -ARMV8_EVENT_ATTR(l2d_cache, ARMV8_PMUV3_PERFCTR_L2D_CACHE); -ARMV8_EVENT_ATTR(l2d_cache_refill, ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL); -ARMV8_EVENT_ATTR(l2d_cache_wb, ARMV8_PMUV3_PERFCTR_L2D_CACHE_WB); -ARMV8_EVENT_ATTR(bus_access, ARMV8_PMUV3_PERFCTR_BUS_ACCESS); -ARMV8_EVENT_ATTR(memory_error, ARMV8_PMUV3_PERFCTR_MEMORY_ERROR); -ARMV8_EVENT_ATTR(inst_spec, ARMV8_PMUV3_PERFCTR_INST_SPEC); -ARMV8_EVENT_ATTR(ttbr_write_retired, ARMV8_PMUV3_PERFCTR_TTBR_WRITE_RETIRED); -ARMV8_EVENT_ATTR(bus_cycles, ARMV8_PMUV3_PERFCTR_BUS_CYCLES); -/* Don't expose the chain event in /sys, since it's useless in isolation */ -ARMV8_EVENT_ATTR(l1d_cache_allocate, ARMV8_PMUV3_PERFCTR_L1D_CACHE_ALLOCATE); -ARMV8_EVENT_ATTR(l2d_cache_allocate, ARMV8_PMUV3_PERFCTR_L2D_CACHE_ALLOCATE); -ARMV8_EVENT_ATTR(br_retired, ARMV8_PMUV3_PERFCTR_BR_RETIRED); -ARMV8_EVENT_ATTR(br_mis_pred_retired, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED_RETIRED); -ARMV8_EVENT_ATTR(stall_frontend, ARMV8_PMUV3_PERFCTR_STALL_FRONTEND); -ARMV8_EVENT_ATTR(stall_backend, ARMV8_PMUV3_PERFCTR_STALL_BACKEND); -ARMV8_EVENT_ATTR(l1d_tlb, ARMV8_PMUV3_PERFCTR_L1D_TLB); -ARMV8_EVENT_ATTR(l1i_tlb, ARMV8_PMUV3_PERFCTR_L1I_TLB); -ARMV8_EVENT_ATTR(l2i_cache, ARMV8_PMUV3_PERFCTR_L2I_CACHE); -ARMV8_EVENT_ATTR(l2i_cache_refill, ARMV8_PMUV3_PERFCTR_L2I_CACHE_REFILL); -ARMV8_EVENT_ATTR(l3d_cache_allocate, ARMV8_PMUV3_PERFCTR_L3D_CACHE_ALLOCATE); -ARMV8_EVENT_ATTR(l3d_cache_refill, ARMV8_PMUV3_PERFCTR_L3D_CACHE_REFILL); -ARMV8_EVENT_ATTR(l3d_cache, ARMV8_PMUV3_PERFCTR_L3D_CACHE); -ARMV8_EVENT_ATTR(l3d_cache_wb, ARMV8_PMUV3_PERFCTR_L3D_CACHE_WB); -ARMV8_EVENT_ATTR(l2d_tlb_refill, ARMV8_PMUV3_PERFCTR_L2D_TLB_REFILL); -ARMV8_EVENT_ATTR(l2i_tlb_refill, ARMV8_PMUV3_PERFCTR_L2I_TLB_REFILL); -ARMV8_EVENT_ATTR(l2d_tlb, ARMV8_PMUV3_PERFCTR_L2D_TLB); -ARMV8_EVENT_ATTR(l2i_tlb, ARMV8_PMUV3_PERFCTR_L2I_TLB); -ARMV8_EVENT_ATTR(remote_access, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS); -ARMV8_EVENT_ATTR(ll_cache, ARMV8_PMUV3_PERFCTR_LL_CACHE); -ARMV8_EVENT_ATTR(ll_cache_miss, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS); -ARMV8_EVENT_ATTR(dtlb_walk, ARMV8_PMUV3_PERFCTR_DTLB_WALK); -ARMV8_EVENT_ATTR(itlb_walk, ARMV8_PMUV3_PERFCTR_ITLB_WALK); -ARMV8_EVENT_ATTR(ll_cache_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_RD); -ARMV8_EVENT_ATTR(ll_cache_miss_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS_RD); -ARMV8_EVENT_ATTR(remote_access_rd, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS_RD); -ARMV8_EVENT_ATTR(sample_pop, ARMV8_SPE_PERFCTR_SAMPLE_POP); -ARMV8_EVENT_ATTR(sample_feed, ARMV8_SPE_PERFCTR_SAMPLE_FEED); -ARMV8_EVENT_ATTR(sample_filtrate, ARMV8_SPE_PERFCTR_SAMPLE_FILTRATE); -ARMV8_EVENT_ATTR(sample_collision, ARMV8_SPE_PERFCTR_SAMPLE_COLLISION); + (&((struct perf_pmu_events_attr[]) { \ + { .attr = __ATTR(name, 0444, armv8pmu_events_sysfs_show, NULL), \ + .id = config, } \ + })[0].attr.attr) static struct attribute *armv8_pmuv3_event_attrs[] = { - &armv8_event_attr_sw_incr.attr.attr, - &armv8_event_attr_l1i_cache_refill.attr.attr, - &armv8_event_attr_l1i_tlb_refill.attr.attr, - &armv8_event_attr_l1d_cache_refill.attr.attr, - &armv8_event_attr_l1d_cache.attr.attr, - &armv8_event_attr_l1d_tlb_refill.attr.attr, - &armv8_event_attr_ld_retired.attr.attr, - &armv8_event_attr_st_retired.attr.attr, - &armv8_event_attr_inst_retired.attr.attr, - &armv8_event_attr_exc_taken.attr.attr, - &armv8_event_attr_exc_return.attr.attr, - &armv8_event_attr_cid_write_retired.attr.attr, - &armv8_event_attr_pc_write_retired.attr.attr, - &armv8_event_attr_br_immed_retired.attr.attr, - &armv8_event_attr_br_return_retired.attr.attr, - &armv8_event_attr_unaligned_ldst_retired.attr.attr, - &armv8_event_attr_br_mis_pred.attr.attr, - &armv8_event_attr_cpu_cycles.attr.attr, - &armv8_event_attr_br_pred.attr.attr, - &armv8_event_attr_mem_access.attr.attr, - &armv8_event_attr_l1i_cache.attr.attr, - &armv8_event_attr_l1d_cache_wb.attr.attr, - &armv8_event_attr_l2d_cache.attr.attr, - &armv8_event_attr_l2d_cache_refill.attr.attr, - &armv8_event_attr_l2d_cache_wb.attr.attr, - &armv8_event_attr_bus_access.attr.attr, - &armv8_event_attr_memory_error.attr.attr, - &armv8_event_attr_inst_spec.attr.attr, - &armv8_event_attr_ttbr_write_retired.attr.attr, - &armv8_event_attr_bus_cycles.attr.attr, - &armv8_event_attr_l1d_cache_allocate.attr.attr, - &armv8_event_attr_l2d_cache_allocate.attr.attr, - &armv8_event_attr_br_retired.attr.attr, - &armv8_event_attr_br_mis_pred_retired.attr.attr, - &armv8_event_attr_stall_frontend.attr.attr, - &armv8_event_attr_stall_backend.attr.attr, - &armv8_event_attr_l1d_tlb.attr.attr, - &armv8_event_attr_l1i_tlb.attr.attr, - &armv8_event_attr_l2i_cache.attr.attr, - &armv8_event_attr_l2i_cache_refill.attr.attr, - &armv8_event_attr_l3d_cache_allocate.attr.attr, - &armv8_event_attr_l3d_cache_refill.attr.attr, - &armv8_event_attr_l3d_cache.attr.attr, - &armv8_event_attr_l3d_cache_wb.attr.attr, - &armv8_event_attr_l2d_tlb_refill.attr.attr, - &armv8_event_attr_l2i_tlb_refill.attr.attr, - &armv8_event_attr_l2d_tlb.attr.attr, - &armv8_event_attr_l2i_tlb.attr.attr, - &armv8_event_attr_remote_access.attr.attr, - &armv8_event_attr_ll_cache.attr.attr, - &armv8_event_attr_ll_cache_miss.attr.attr, - &armv8_event_attr_dtlb_walk.attr.attr, - &armv8_event_attr_itlb_walk.attr.attr, - &armv8_event_attr_ll_cache_rd.attr.attr, - &armv8_event_attr_ll_cache_miss_rd.attr.attr, - &armv8_event_attr_remote_access_rd.attr.attr, - &armv8_event_attr_sample_pop.attr.attr, - &armv8_event_attr_sample_feed.attr.attr, - &armv8_event_attr_sample_filtrate.attr.attr, - &armv8_event_attr_sample_collision.attr.attr, + ARMV8_EVENT_ATTR(sw_incr, ARMV8_PMUV3_PERFCTR_SW_INCR), + ARMV8_EVENT_ATTR(l1i_cache_refill, ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL), + ARMV8_EVENT_ATTR(l1i_tlb_refill, ARMV8_PMUV3_PERFCTR_L1I_TLB_REFILL), + ARMV8_EVENT_ATTR(l1d_cache_refill, ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL), + ARMV8_EVENT_ATTR(l1d_cache, ARMV8_PMUV3_PERFCTR_L1D_CACHE), + ARMV8_EVENT_ATTR(l1d_tlb_refill, ARMV8_PMUV3_PERFCTR_L1D_TLB_REFILL), + ARMV8_EVENT_ATTR(ld_retired, ARMV8_PMUV3_PERFCTR_LD_RETIRED), + ARMV8_EVENT_ATTR(st_retired, ARMV8_PMUV3_PERFCTR_ST_RETIRED), + ARMV8_EVENT_ATTR(inst_retired, ARMV8_PMUV3_PERFCTR_INST_RETIRED), + ARMV8_EVENT_ATTR(exc_taken, ARMV8_PMUV3_PERFCTR_EXC_TAKEN), + ARMV8_EVENT_ATTR(exc_return, ARMV8_PMUV3_PERFCTR_EXC_RETURN), + ARMV8_EVENT_ATTR(cid_write_retired, ARMV8_PMUV3_PERFCTR_CID_WRITE_RETIRED), + ARMV8_EVENT_ATTR(pc_write_retired, ARMV8_PMUV3_PERFCTR_PC_WRITE_RETIRED), + ARMV8_EVENT_ATTR(br_immed_retired, ARMV8_PMUV3_PERFCTR_BR_IMMED_RETIRED), + ARMV8_EVENT_ATTR(br_return_retired, ARMV8_PMUV3_PERFCTR_BR_RETURN_RETIRED), + ARMV8_EVENT_ATTR(unaligned_ldst_retired, ARMV8_PMUV3_PERFCTR_UNALIGNED_LDST_RETIRED), + ARMV8_EVENT_ATTR(br_mis_pred, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED), + ARMV8_EVENT_ATTR(cpu_cycles, ARMV8_PMUV3_PERFCTR_CPU_CYCLES), + ARMV8_EVENT_ATTR(br_pred, ARMV8_PMUV3_PERFCTR_BR_PRED), + ARMV8_EVENT_ATTR(mem_access, ARMV8_PMUV3_PERFCTR_MEM_ACCESS), + ARMV8_EVENT_ATTR(l1i_cache, ARMV8_PMUV3_PERFCTR_L1I_CACHE), + ARMV8_EVENT_ATTR(l1d_cache_wb, ARMV8_PMUV3_PERFCTR_L1D_CACHE_WB), + ARMV8_EVENT_ATTR(l2d_cache, ARMV8_PMUV3_PERFCTR_L2D_CACHE), + ARMV8_EVENT_ATTR(l2d_cache_refill, ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL), + ARMV8_EVENT_ATTR(l2d_cache_wb, ARMV8_PMUV3_PERFCTR_L2D_CACHE_WB), + ARMV8_EVENT_ATTR(bus_access, ARMV8_PMUV3_PERFCTR_BUS_ACCESS), + ARMV8_EVENT_ATTR(memory_error, ARMV8_PMUV3_PERFCTR_MEMORY_ERROR), + ARMV8_EVENT_ATTR(inst_spec, ARMV8_PMUV3_PERFCTR_INST_SPEC), + ARMV8_EVENT_ATTR(ttbr_write_retired, ARMV8_PMUV3_PERFCTR_TTBR_WRITE_RETIRED), + ARMV8_EVENT_ATTR(bus_cycles, ARMV8_PMUV3_PERFCTR_BUS_CYCLES), + /* Don't expose the chain event in /sys, since it's useless in isolation */ + ARMV8_EVENT_ATTR(l1d_cache_allocate, ARMV8_PMUV3_PERFCTR_L1D_CACHE_ALLOCATE), + ARMV8_EVENT_ATTR(l2d_cache_allocate, ARMV8_PMUV3_PERFCTR_L2D_CACHE_ALLOCATE), + ARMV8_EVENT_ATTR(br_retired, ARMV8_PMUV3_PERFCTR_BR_RETIRED), + ARMV8_EVENT_ATTR(br_mis_pred_retired, ARMV8_PMUV3_PERFCTR_BR_MIS_PRED_RETIRED), + ARMV8_EVENT_ATTR(stall_frontend, ARMV8_PMUV3_PERFCTR_STALL_FRONTEND), + ARMV8_EVENT_ATTR(stall_backend, ARMV8_PMUV3_PERFCTR_STALL_BACKEND), + ARMV8_EVENT_ATTR(l1d_tlb, ARMV8_PMUV3_PERFCTR_L1D_TLB), + ARMV8_EVENT_ATTR(l1i_tlb, ARMV8_PMUV3_PERFCTR_L1I_TLB), + ARMV8_EVENT_ATTR(l2i_cache, ARMV8_PMUV3_PERFCTR_L2I_CACHE), + ARMV8_EVENT_ATTR(l2i_cache_refill, ARMV8_PMUV3_PERFCTR_L2I_CACHE_REFILL), + ARMV8_EVENT_ATTR(l3d_cache_allocate, ARMV8_PMUV3_PERFCTR_L3D_CACHE_ALLOCATE), + ARMV8_EVENT_ATTR(l3d_cache_refill, ARMV8_PMUV3_PERFCTR_L3D_CACHE_REFILL), + ARMV8_EVENT_ATTR(l3d_cache, ARMV8_PMUV3_PERFCTR_L3D_CACHE), + ARMV8_EVENT_ATTR(l3d_cache_wb, ARMV8_PMUV3_PERFCTR_L3D_CACHE_WB), + ARMV8_EVENT_ATTR(l2d_tlb_refill, ARMV8_PMUV3_PERFCTR_L2D_TLB_REFILL), + ARMV8_EVENT_ATTR(l2i_tlb_refill, ARMV8_PMUV3_PERFCTR_L2I_TLB_REFILL), + ARMV8_EVENT_ATTR(l2d_tlb, ARMV8_PMUV3_PERFCTR_L2D_TLB), + ARMV8_EVENT_ATTR(l2i_tlb, ARMV8_PMUV3_PERFCTR_L2I_TLB), + ARMV8_EVENT_ATTR(remote_access, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS), + ARMV8_EVENT_ATTR(ll_cache, ARMV8_PMUV3_PERFCTR_LL_CACHE), + ARMV8_EVENT_ATTR(ll_cache_miss, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS), + ARMV8_EVENT_ATTR(dtlb_walk, ARMV8_PMUV3_PERFCTR_DTLB_WALK), + ARMV8_EVENT_ATTR(itlb_walk, ARMV8_PMUV3_PERFCTR_ITLB_WALK), + ARMV8_EVENT_ATTR(ll_cache_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_RD), + ARMV8_EVENT_ATTR(ll_cache_miss_rd, ARMV8_PMUV3_PERFCTR_LL_CACHE_MISS_RD), + ARMV8_EVENT_ATTR(remote_access_rd, ARMV8_PMUV3_PERFCTR_REMOTE_ACCESS_RD), + ARMV8_EVENT_ATTR(sample_pop, ARMV8_SPE_PERFCTR_SAMPLE_POP), + ARMV8_EVENT_ATTR(sample_feed, ARMV8_SPE_PERFCTR_SAMPLE_FEED), + ARMV8_EVENT_ATTR(sample_filtrate, ARMV8_SPE_PERFCTR_SAMPLE_FILTRATE), + ARMV8_EVENT_ATTR(sample_collision, ARMV8_SPE_PERFCTR_SAMPLE_COLLISION), NULL, };
For each PMU event, there is a ARMV8_EVENT_ATTR(xx, XX) and &armv8_event_attr_xx.attr.attr. Let's redefine the ARMV8_EVENT_ATTR to simplify the armv8_pmuv3_event_attrs. Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> --- arch/arm64/kernel/perf_event.c | 189 ++++++++++++++--------------------------- 1 file changed, 65 insertions(+), 124 deletions(-)