Re: [PATCH 2/2] perf arm-spe: Parse more SPE fields and store source

From: Ali Saidi <alisaidi@amazon.com>
To: <german.gomez@arm.com>
Cc: <acme@kernel.org>, <alexander.shishkin@linux.intel.com>,
	<alisaidi@amazon.com>, <andrew.kilroy@arm.com>,
	<benh@kernel.crashing.org>, <james.clark@arm.com>,
	<john.garry@huawei.com>, <jolsa@redhat.com>, <leo.yan@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-perf-users@vger.kernel.org>, <mark.rutland@arm.com>,
	<mathieu.poirier@linaro.org>, <mingo@redhat.com>,
	<namhyung@kernel.org>, <peterz@infradead.org>, <will@kernel.org>
Subject: Re: [PATCH 2/2] perf arm-spe: Parse more SPE fields and store source
Date: Fri, 28 Jan 2022 21:02:45 +0000	[thread overview]
Message-ID: <20220128210245.4628-1-alisaidi@amazon.com> (raw)
In-Reply-To: <b1b3697d-4a0a-d041-5cbd-e08fec9e658c@arm.com>

Hi German,

On 28/01/2022 19:20, German Gomez wrote:
>Hi Ali,
>
>On 25/01/2022 19:20, Ali Saidi wrote:
>> Decode more SPE events and op types to allow for processing by perf
>> scripts. For example looking for branches which may indicate candidates
>> for conversion to a CSEL, store exclusives that are candidates for
>> conversion to LSE atomics and record the source information for memory
>> ops.
>> [snip]
>> +				if (SPE_OP_PKT_IS_LDST_ATOMIC(payload)) {
>> +					if (payload & SPE_OP_PKT_AT)
>> +						decoder->record.op |= ARM_SPE_LDST_ATOMIC;
>
>In "utils/arm-spe.c" we check "if (record->op == ARM_SPE_LD)" so this
>ORing could break some of the generated samples.

Yep, you're correct. Interestingly I can only find one use of record->op using
equivalence instead of a logical and so perhaps it's best to fix this one use.

...
>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> index 69b31084d6be..113e427afe99 100644
>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
>> @@ -22,11 +22,18 @@ enum arm_spe_sample_type {
>>  	ARM_SPE_TLB_MISS	= 1 << 5,
>>  	ARM_SPE_BRANCH_MISS	= 1 << 6,
>>  	ARM_SPE_REMOTE_ACCESS	= 1 << 7,
>> +	ARM_SPE_BR_NOT_TAKEN	= 1 << 8,
>
>Can you rename it to ARM_SPE_BRANCH_NOT_TAKEN for consistency?

No problem

>
>>  };
>>  
>>  enum arm_spe_op_type {
>>  	ARM_SPE_LD		= 1 << 0,
>>  	ARM_SPE_ST		= 1 << 1,
>> +	ARM_SPE_LDST_EXCL	= 1 << 2,
>> +	ARM_SPE_LDST_ATOMIC	= 1 << 3,
>> +	ARM_SPE_LDST_ACQREL	= 1 << 4,
>> +	ARM_SPE_BR		= 1 << 5,
>> +	ARM_SPE_BR_COND		= 1 << 6,
>> +	ARM_SPE_BR_IND		= 1 << 7,
>
>I'm not sure if we should keep everything in one enum/bitmask. I'm also
>looking at adding more of the data from the packets to the record, and
>considering refactoring the record structure. I'll share here when I
>have something.

One straight forward way to do this would be to make it a u16 field that was 
SPE operation-type header and operation-type payload with some accessors instead
of trying to re-encode the operation type into a new format.

Thanks,
Ali