Re: [PATCH] perf tools: Fix bug when recording SPE and non SPE events

[James Clark]

Hi Leo,

Do you mean that you would never expect there to be more than one SPE file like /sys/bus/event_source/devices/arm_spe_0?

If that is the case then do you know why there is still a number appended to the file?


Thanks
James

On 23/12/2019 03:48, Leo Yan wrote:
> Hi James,
> 
> On Fri, Dec 20, 2019 at 11:05:25AM +0000, James Clark wrote:
>> This patch fixes an issue when non Arm SPE events are specified after an
>> Arm SPE event. In that case, perf will exit with an error code and not
>> produce a record file. This is because a loop index is used to store the
>> location of the relevant Arm SPE PMU, but if non SPE PMUs follow, that
>> index will be overwritten. Fix this issue by saving the PMU into a
>> variable instead of using the index, and also add an error message.
>>
>> Before the fix:
>>     ./perf record -e arm_spe/ts_enable=1/ -e branch-misses ls; echo $?
>>     237
>>
>> After the fix:
>>     ./perf record -e arm_spe/ts_enable=1/ -e branch-misses ls; echo $?
>>     ...
>>     0
> 
> Just bring up a question related with PMU event registration.  Let's
> see the DT binding in arch/arm64/boot/dts/arm/fvp-base-revc.dts:
> 
>          spe-pmu {
>                  compatible = "arm,statistical-profiling-extension-v1";
>                  interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_HIGH>;
>          };
> 
> 
> Now SPE registers PMU event for every CPU; seem to me, though SPE is an
> IP binding per CPU, it should register into perf framework with single
> pmu event, ARM's PMU/ETM/IntelPT all use this way to regsiter PMU event;
> this can allow perf tool logic to be more neat.
> 
> After the driver changes to use single PMU registration, the perf tool
> code can be changed to use simple way to find perf_pmu and this data
> structure can be not bound to a specific CPU.  Finally, this bug can
> be smoothly dismissed.
> 
> Thanks,
> Leo
> 
>> Signed-off-by: James Clark <james.clark@xxxxxxx>
>> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
>> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>> Cc: Igor Lubashev <ilubashe@xxxxxxxxxx>
>> ---
>>  tools/perf/arch/arm/util/auxtrace.c  | 10 +++++-----
>>  tools/perf/arch/arm64/util/arm-spe.c |  1 +
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
>> index 0a6e75b8777a..230f03b622e1 100644
>> --- a/tools/perf/arch/arm/util/auxtrace.c
>> +++ b/tools/perf/arch/arm/util/auxtrace.c
>> @@ -54,9 +54,9 @@ struct auxtrace_record
>>  *auxtrace_record__init(struct evlist *evlist, int *err)
>>  {
>>  	struct perf_pmu	*cs_etm_pmu;
>> +	struct perf_pmu *arm_spe_pmu = NULL;
>>  	struct evsel *evsel;
>>  	bool found_etm = false;
>> -	bool found_spe = false;
>>  	static struct perf_pmu **arm_spe_pmus = NULL;
>>  	static int nr_spes = 0;
>>  	int i = 0;
>> @@ -79,13 +79,13 @@ struct auxtrace_record
>>  
>>  		for (i = 0; i < nr_spes; i++) {
>>  			if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
>> -				found_spe = true;
>> +				arm_spe_pmu = arm_spe_pmus[i];
>>  				break;
>>  			}
>>  		}
>>  	}
>>  
>> -	if (found_etm && found_spe) {
>> +	if (found_etm && arm_spe_pmu) {
>>  		pr_err("Concurrent ARM Coresight ETM and SPE operation not currently supported\n");
>>  		*err = -EOPNOTSUPP;
>>  		return NULL;
>> @@ -95,8 +95,8 @@ struct auxtrace_record
>>  		return cs_etm_record_init(err);
>>  
>>  #if defined(__aarch64__)
>> -	if (found_spe)
>> -		return arm_spe_recording_init(err, arm_spe_pmus[i]);
>> +	if (arm_spe_pmu)
>> +		return arm_spe_recording_init(err, arm_spe_pmu);
>>  #endif
>>  
>>  	/*
>> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
>> index eba6541ec0f1..b7d17d8724df 100644
>> --- a/tools/perf/arch/arm64/util/arm-spe.c
>> +++ b/tools/perf/arch/arm64/util/arm-spe.c
>> @@ -178,6 +178,7 @@ struct auxtrace_record *arm_spe_recording_init(int *err,
>>  	struct arm_spe_recording *sper;
>>  
>>  	if (!arm_spe_pmu) {
>> +		pr_err("Attempted to initialise null SPE PMU\n");
>>  		*err = -ENODEV;
>>  		return NULL;
>>  	}
>> -- 
>> 2.24.0
>>