Re: [PATCH v3 1/9] arm64: perf: avoid PMXEV* indirection

From: Julien Thierry <julien.thierry@arm.com>
To: Steven Price <steven.price@arm.com>,
	linux-arm-kernel@lists.infradead.org
Cc: mark.rutland@arm.com, peterz@infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	jolsa@redhat.com, will.deacon@arm.com, acme@kernel.org,
	alexander.shishkin@linux.intel.com, mingo@redhat.com,
	namhyung@kernel.org, liwei391@huawei.com
Subject: Re: [PATCH v3 1/9] arm64: perf: avoid PMXEV* indirection
Date: Wed, 10 Jul 2019 12:01:32 +0100	[thread overview]
Message-ID: <0e1169eb-1a2a-eaa3-82b2-74b263db527d@arm.com> (raw)
In-Reply-To: <72820d6b-145c-c7dd-b285-c3d3b8acd103@arm.com>

On 10/07/2019 11:57, Steven Price wrote:
> On 08/07/2019 15:32, Julien Thierry wrote:
>> From: Mark Rutland <mark.rutland@arm.com>
>>
>> Currently we access the counter registers and their respective type
>> registers indirectly. This requires us to write to PMSELR, issue an ISB,
>> then access the relevant PMXEV* registers.
>>
>> This is unfortunate, because:
>>
>> * Under virtualization, accessing one registers requires two traps to
>>   the hypervisor, even though we could access the register directly with
>>   a single trap.
>>
>> * We have to issue an ISB which we could otherwise avoid the cost of.
>>
>> * When we use NMIs, the NMI handler will have to save/restore the select
>>   register in case the code it preempted was attempting to access a
>>   counter or its type register.
>>
>> We can avoid these issues by directly accessing the relevant registers.
>> This patch adds helpers to do so.
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> [Julien T.: Don't inline read/write functions to avoid big code-size
>> 	increase, remove unused read_pmevtypern function,
>> 	fix counter index issue.]
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> ---
>>  arch/arm64/kernel/perf_event.c | 96 ++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index 96e90e2..7759f8a 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -369,6 +369,77 @@ static inline bool armv8pmu_event_is_chained(struct perf_event *event)
>>  #define	ARMV8_IDX_TO_COUNTER(x)	\
>>  	(((x) - ARMV8_IDX_COUNTER0) & ARMV8_PMU_COUNTER_MASK)
>>
>> +/*
>> + * This code is really good
>> + */
>> +
>> +#define PMEVN_CASE(n, case_macro) \
>> +	case n: case_macro(n); break
>> +
>> +#define PMEVN_SWITCH(x, case_macro)				\
>> +	do {							\
>> +		switch (x) {					\
>> +		PMEVN_CASE(0,  case_macro);			\
>> +		PMEVN_CASE(1,  case_macro);			\
>> +		PMEVN_CASE(2,  case_macro);			\
>> +		PMEVN_CASE(3,  case_macro);			\
>> +		PMEVN_CASE(4,  case_macro);			\
>> +		PMEVN_CASE(5,  case_macro);			\
>> +		PMEVN_CASE(6,  case_macro);			\
>> +		PMEVN_CASE(7,  case_macro);			\
>> +		PMEVN_CASE(8,  case_macro);			\
>> +		PMEVN_CASE(9,  case_macro);			\
>> +		PMEVN_CASE(10, case_macro);			\
>> +		PMEVN_CASE(11, case_macro);			\
>> +		PMEVN_CASE(12, case_macro);			\
>> +		PMEVN_CASE(13, case_macro);			\
>> +		PMEVN_CASE(14, case_macro);			\
>> +		PMEVN_CASE(15, case_macro);			\
>> +		PMEVN_CASE(16, case_macro);			\
>> +		PMEVN_CASE(17, case_macro);			\
>> +		PMEVN_CASE(18, case_macro);			\
>> +		PMEVN_CASE(19, case_macro);			\
> 
> Is 20 missing on purpose?
> 

That would have been fun to debug. Well spotted!

I'll fix it in the next version.

Thanks,

-- 
Julien Thierry

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