From: "André Przywara" <andre.przywara@arm.com>
To: Leo Yan <leo.yan@linaro.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
Wei Li <liwei391@huawei.com>, James Clark <james.clark@arm.com>,
Dave Martin <Dave.Martin@arm.com>,
linux-kernel@vger.kernel.org, Al Grant <Al.Grant@arm.com>
Subject: Re: [PATCH v2 10/14] perf arm-spe: Refactor event type handling
Date: Tue, 20 Oct 2020 22:54:16 +0100 [thread overview]
Message-ID: <2605044c-8172-00cf-e924-ece5a0b70e2c@arm.com> (raw)
In-Reply-To: <20200929133917.9224-11-leo.yan@linaro.org>
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@linaro.org>
> ---
> .../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 prev parent reply other threads:[~2020-10-20 21:55 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-29 13:39 [PATCH v2 00/14] perf arm-spe: Refactor decoding & dumping flow Leo Yan
2020-09-29 13:39 ` [PATCH v2 01/14] perf arm-spe: Include bitops.h for BIT() macro Leo Yan
2020-10-08 13:44 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 02/14] perf arm-spe: Fix a typo in comment Leo Yan
2020-10-08 13:44 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 03/14] perf arm-spe: Refactor payload length calculation Leo Yan
2020-10-08 13:44 ` André Przywara
2020-10-12 0:21 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 04/14] perf arm-spe: Fix packet length handling Leo Yan
2020-10-08 13:45 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 05/14] perf arm-spe: Refactor printing string to buffer Leo Yan
2020-10-08 13:46 ` André Przywara
2020-10-12 0:29 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 06/14] perf arm-spe: Refactor packet header parsing Leo Yan
2020-10-08 19:49 ` André Przywara
2020-10-12 1:00 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 07/14] perf arm-spe: Refactor address packet handling Leo Yan
2020-10-19 9:01 ` André Przywara
2020-10-19 10:41 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 08/14] perf arm-spe: Refactor context " Leo Yan
2020-10-20 21:53 ` André Przywara
2020-09-29 13:39 ` [PATCH v2 09/14] perf arm-spe: Refactor counter " Leo Yan
2020-10-20 21:53 ` André Przywara
2020-10-21 3:52 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 10/14] perf arm-spe: Refactor event type handling Leo Yan
2020-10-20 21:54 ` André Przywara [this message]
2020-10-21 4:54 ` Leo Yan
2020-10-21 9:20 ` André Przywara
2020-10-21 10:13 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 11/14] perf arm-spe: Refactor operation packet handling Leo Yan
2020-09-29 13:39 ` [PATCH v2 12/14] perf arm-spe: Add more sub classes for operation packet Leo Yan
2020-10-20 21:54 ` André Przywara
2020-10-21 5:16 ` Leo Yan
2020-09-29 13:39 ` [PATCH v2 13/14] perf arm_spe: Decode memory tagging properties Leo Yan
2020-09-29 13:39 ` [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE Leo Yan
2020-10-20 21:54 ` André Przywara
2020-10-21 5:10 ` Leo Yan
2020-10-21 9:26 ` André Przywara
2020-10-21 10:17 ` Leo Yan
2020-10-21 14:53 ` André Przywara
2020-10-22 0:44 ` Leo Yan
2020-10-13 14:53 ` [PATCH v2 00/14] perf arm-spe: Refactor decoding & dumping flow Arnaldo Carvalho de Melo
2020-10-13 15:19 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2605044c-8172-00cf-e924-ece5a0b70e2c@arm.com \
--to=andre.przywara@arm.com \
--cc=Al.Grant@arm.com \
--cc=Dave.Martin@arm.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=james.clark@arm.com \
--cc=jolsa@redhat.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liwei391@huawei.com \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).