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

From: Shaokun Zhang <zhangshaokun@hisilicon.com>
To: Richard Henderson <richard.henderson@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>
Cc: Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH] arm64: perf: Simplify the ARMv8 PMUv3 event attributes
Date: Thu, 31 Oct 2019 16:45:52 +0800	[thread overview]
Message-ID: <a503b65c-3bf1-83d5-08ba-3f25c880ba97@hisilicon.com> (raw)
In-Reply-To: <b603fc49-01f5-b5b4-7c2c-5f0bc53b6fc1@linaro.org>

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~
> 
> 

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