Re: [PATCH v2 10/14] perf arm-spe: Refactor event type handling

From: André Przywara
Date: Tue Oct 20 2020 - 17:55:11 EST

Next message: André Przywara: "Re: [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE"
Previous message: André Przywara: "Re: [PATCH v2 09/14] perf arm-spe: Refactor counter packet handling"
Next in thread: Leo Yan: "Re: [PATCH v2 10/14] perf arm-spe: Refactor event type handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 29/09/2020 14:39, Leo Yan wrote:

Hi,

> Use macros instead of the enum values for event types, this is more
> directive and without bit shifting when parse packet.
>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
> .../util/arm-spe-decoder/arm-spe-decoder.c | 16 +++++++-------
> .../util/arm-spe-decoder/arm-spe-decoder.h | 17 --------------
> .../arm-spe-decoder/arm-spe-pkt-decoder.c | 22 +++++++++----------
> .../arm-spe-decoder/arm-spe-pkt-decoder.h | 16 ++++++++++++++
> 4 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> index 9d3de163d47c..ac66e7f42a58 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> @@ -168,31 +168,31 @@ static int arm_spe_read_record(struct arm_spe_decoder *decoder)
> case ARM_SPE_OP_TYPE:
> break;
> case ARM_SPE_EVENTS:
> - if (payload & BIT(EV_L1D_REFILL))
> + if (payload & SPE_EVT_PKT_L1D_REFILL)

Not sure this (and the others below) are an improvement? I liked the
enum below, and reading BIT() here tells me that it's a bitmask.

> decoder->record.type |= ARM_SPE_L1D_MISS;
>
> - if (payload & BIT(EV_L1D_ACCESS))
> + if (payload & SPE_EVT_PKT_L1D_ACCESS)
> decoder->record.type |= ARM_SPE_L1D_ACCESS;
>
> - if (payload & BIT(EV_TLB_WALK))
> + if (payload & SPE_EVT_PKT_TLB_WALK)
> decoder->record.type |= ARM_SPE_TLB_MISS;
>
> - if (payload & BIT(EV_TLB_ACCESS))
> + if (payload & SPE_EVT_PKT_TLB_ACCESS)
> decoder->record.type |= ARM_SPE_TLB_ACCESS;
>
> if ((idx == 2 || idx == 4 || idx == 8) &&
> - (payload & BIT(EV_LLC_MISS)))
> + (payload & SPE_EVT_PKT_LLC_MISS))
> decoder->record.type |= ARM_SPE_LLC_MISS;
>
> if ((idx == 2 || idx == 4 || idx == 8) &&
> - (payload & BIT(EV_LLC_ACCESS)))
> + (payload & SPE_EVT_PKT_LLC_ACCESS))
> decoder->record.type |= ARM_SPE_LLC_ACCESS;
>
> if ((idx == 2 || idx == 4 || idx == 8) &&
> - (payload & BIT(EV_REMOTE_ACCESS)))
> + (payload & SPE_EVT_PKT_REMOTE_ACCESS))
> decoder->record.type |= ARM_SPE_REMOTE_ACCESS;
>
> - if (payload & BIT(EV_MISPRED))
> + if (payload & SPE_EVT_PKT_MISPREDICTED)
> decoder->record.type |= ARM_SPE_BRANCH_MISS;
>
> break;
> 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 a5111a8d4360..24727b8ca7ff 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.h
> @@ -13,23 +13,6 @@
>
> #include "arm-spe-pkt-decoder.h"
>
> -enum arm_spe_events {
> - EV_EXCEPTION_GEN = 0,
> - EV_RETIRED = 1,
> - EV_L1D_ACCESS = 2,
> - EV_L1D_REFILL = 3,
> - EV_TLB_ACCESS = 4,
> - EV_TLB_WALK = 5,
> - EV_NOT_TAKEN = 6,
> - EV_MISPRED = 7,
> - EV_LLC_ACCESS = 8,
> - EV_LLC_MISS = 9,
> - EV_REMOTE_ACCESS = 10,
> - EV_ALIGNMENT = 11,
> - EV_PARTIAL_PREDICATE = 17,
> - EV_EMPTY_PREDICATE = 18,
> -};

So what about keeping this, but moving it into the other header file?
coding-style.rst says: "Enums are preferred when defining several
related constants."

> -
> enum arm_spe_sample_type {
> ARM_SPE_L1D_ACCESS = 1 << 0,
> ARM_SPE_L1D_MISS = 1 << 1,
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index ed0f4c74dfc5..b8f343320abf 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -284,58 +284,58 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> if (ret < 0)
> return ret;
>
> - if (payload & 0x1) {
> + if (payload & SPE_EVT_PKT_GEN_EXCEPTION) {

Having the bitmask here directly is indeed not very nice and error
prone. But I would rather see the above solution:
if (payload & BIT(EV_EXCEPTION_GEN)) {

> ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x2) {
> + if (payload & SPE_EVT_PKT_ARCH_RETIRED) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x4) {
> + if (payload & SPE_EVT_PKT_L1D_ACCESS) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x8) {
> + if (payload & SPE_EVT_PKT_L1D_REFILL) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x10) {
> + if (payload & SPE_EVT_PKT_TLB_ACCESS) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x20) {
> + if (payload & SPE_EVT_PKT_TLB_WALK) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x40) {
> + if (payload & SPE_EVT_PKT_NOT_TAKEN) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x80) {
> + if (payload & SPE_EVT_PKT_MISPREDICTED) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
> if (ret < 0)
> return ret;
> }
> if (idx > 1) {

Do you know what the purpose of this comparison is? Surely payload would
not contain more bits than would fit in "idx" bytes? So is this some
attempt of an optimisation? If so, I doubt it's really useful, the
compiler might find a smarter solution to the problem. Just continuing
with the bit mask comparison would make it look nicer, I think.

Cheers,
Andre

> - if (payload & 0x100) {
> + if (payload & SPE_EVT_PKT_LLC_ACCESS) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-ACCESS");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x200) {
> + if (payload & SPE_EVT_PKT_LLC_MISS) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " LLC-REFILL");
> if (ret < 0)
> return ret;
> }
> - if (payload & 0x400) {
> + if (payload & SPE_EVT_PKT_REMOTE_ACCESS) {
> ret = arm_spe_pkt_snprintf(&buf, &blen, " REMOTE-ACCESS");
> if (ret < 0)
> return ret;
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> index 18667a63f5ba..e9a88cf685bb 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> @@ -98,6 +98,22 @@ struct arm_spe_pkt {
> #define SPE_CNT_PKT_HDR_EXT_INDEX_MASK GENMASK_ULL(1, 0)
> #define SPE_CNT_PKT_HDR_EXT_INDEX_SHIFT (3)
>
> +/* Event packet payload */
> +#define SPE_EVT_PKT_SVE_EMPTY_PREDICATE BIT(18)
> +#define SPE_EVT_PKT_SVE_PARTIAL_PREDICATE BIT(17)
> +#define SPE_EVT_PKT_ALIGNMENT BIT(11)
> +#define SPE_EVT_PKT_REMOTE_ACCESS BIT(10)
> +#define SPE_EVT_PKT_LLC_MISS BIT(9)
> +#define SPE_EVT_PKT_LLC_ACCESS BIT(8)
> +#define SPE_EVT_PKT_MISPREDICTED BIT(7)
> +#define SPE_EVT_PKT_NOT_TAKEN BIT(6)
> +#define SPE_EVT_PKT_TLB_WALK BIT(5)
> +#define SPE_EVT_PKT_TLB_ACCESS BIT(4)
> +#define SPE_EVT_PKT_L1D_REFILL BIT(3)
> +#define SPE_EVT_PKT_L1D_ACCESS BIT(2)
> +#define SPE_EVT_PKT_ARCH_RETIRED BIT(1)
> +#define SPE_EVT_PKT_GEN_EXCEPTION BIT(0)
> +
> const char *arm_spe_pkt_name(enum arm_spe_pkt_type);
>
> int arm_spe_get_packet(const unsigned char *buf, size_t len,
>

Next message: André Przywara: "Re: [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE"
Previous message: André Przywara: "Re: [PATCH v2 09/14] perf arm-spe: Refactor counter packet handling"
Next in thread: Leo Yan: "Re: [PATCH v2 10/14] perf arm-spe: Refactor event type handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]