Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes

[Mark Rutland <mark.rutland@arm.com>]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel