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

[Leo Yan]

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
>