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

[Leo Yan <leo.yan@linaro.org>]

Hi Suzuki,

On Mon, Jan 13, 2020 at 12:27:38PM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo,
> 
> 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
> 
> Do you mean "SPE PMU" here ? SPE is different from ETM, where the trace
> data is micro-architecture dependent. And thus you cannot mix the trace
> on different CPUs with different micro-archs.

Understood that SPE is micro-architecture dependent.

Usually, we should register PMU event once for the same SPE version and
CPUs can create multiple instances.  My concern at here is the PMU event
is regsitered for multiple times for the same SPE version.  Please
correct me if I misunderstand.

> As such I don't see any issue with this patch.

Regard this patch, it does fix the issue if based on current PMU event
registration; so it's okay for me to merge it if now it's not necessary
to change PMU event registration.

Thanks,
Leo Yan