Messages in this thread | | | From | Ali Saidi <> | Subject | Re: [PATCH 2/2] perf arm-spe: Parse more SPE fields and store source | Date | Fri, 28 Jan 2022 21:02:45 +0000 |
| |
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
| |