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

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

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.

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