From: Dave Martin <Dave.Martin@arm.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: "Mark Rutland" <mark.rutland@arm.com>,
"Al Grant" <Al.Grant@arm.com>,
linux-arm-kernel@lists.infradead.org,
"Suzuki K Poulose" <suzuki.poulose@arm.com>,
"Peter Zijlstra" <peterz@infradead.org>,
"André Przywara" <andre.przywara@arm.com>,
"Jiri Olsa" <jolsa@redhat.com>,
linux-kernel@vger.kernel.org,
"Arnaldo Carvalho de Melo" <acme@kernel.org>,
"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
"Ingo Molnar" <mingo@redhat.com>,
"James Clark" <james.clark@arm.com>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Namhyung Kim" <namhyung@kernel.org>,
"Will Deacon" <will@kernel.org>,
"Tan Xiaojun" <tanxiaojun@huawei.com>,
"Wei Li" <liwei391@huawei.com>
Subject: Re: [PATCH 5/5] perf: arm_spe: Decode SVE events
Date: Mon, 5 Oct 2020 11:15:44 +0100 [thread overview]
Message-ID: <20201005101541.GQ6642@arm.com> (raw)
In-Reply-To: <20200930110453.GB9968@leoy-ThinkPad-X240s>
On Wed, Sep 30, 2020 at 07:04:53PM +0800, Leo Yan wrote:
[...]
> > > > > >> 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 a033f34846a6..f0c369259554 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
> > > > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > > > >> }
> > > > > >> case ARM_SPE_OP_TYPE:
> > > > > >> switch (idx) {
> > > > > >> - case 0: return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > > > >> + case 0: {
> > > > > >> + size_t blen = buf_len;
> > > > > >> +
> > > > > >> + if ((payload & 0x89) == 0x08) {
> > > > > >> + ret = snprintf(buf, buf_len, "SVE");
> > > > > >> + buf += ret;
> > > > > >> + blen -= ret;
> > > > > >
> > > > > > (Nit: can ret be < 0 ? I've never been 100% clear on this myself for
> > > > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > > > a lready that I guess just go with the flow.)
> > > > >
> > > > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > > > this on every call to snprintf would push this current code over the
> > > > > edge - and I cowardly avoided a refactoring ;-)
> > > > >
> > > > > Please note that his is perf userland, and also we are printing constant
> > > > > strings here.
> > > > > Although admittedly this starts to sounds like an excuse now ...
> > > > >
> > > > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > > > up as a helper, rather than having to be repeated all over the place.
> > > > >
> > > > > Yes, I was hoping nobody would notice ;-)
> > > >
> > > > It's probably not worth losing sleep over.
> > > >
> > > > snprintf(3) says, under NOTES:
> > > >
> > > > Until glibc 2.0.6, they would return -1 when the output was
> > > > truncated.
> > > >
> > > > which is probably ancient enough history that we don't care. C11 does
> > > > say that a negative return value can happen "if an encoding error
> > > > occurred". _Probably_ not a problem if perf tool never calls
> > > > setlocale(), but ...
> > >
> > > I have one patch which tried to fix the snprintf+increment sequence
> > > [1], to be honest, the change seems urgly for me. I agree it's better
> > > to use a helper to wrap up.
> > >
> > > [1] https://lore.kernel.org/patchwork/patch/1288410/
> >
> > Sure, putting explicit checks all over the place makes a lot of noise in
> > the code.
> >
> > I was wondering whether something along the following lines would work:
> >
> > /* ... */
> >
> > if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
> > buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
> > if (payload & SVE_EVT_PKT_ARCH_RETIRED)
> > buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
> > if (payload & SVE_EVT_PKT_L1D_ACCESS)
> > buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");
> >
> > /* ... */
> >
> > if (ret)
> > return ret;
> >
> > [...]
>
> I have sent out the patch v2 [1] and Cc'ed you; I used a similiar API
> definition with your suggestion:
>
> static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
> const char *fmt, ...)
>
> Only a difference is when return from arm_spe_pkt_snprintf(), will check
> the return value and directly bail out when detect failure. Your input
> will be considered for next spin.
>
> > Best to keep such refactoring independent of this series though.
>
> Yeah, the patch set [2] is quite heavy; after get some reviewing,
> maybe need to consider to split into 2 or even 3 small patch sets.
>
> Thanks a lot for your suggestions!
>
> Leo
No problem, your approach seems reasonable to me.
Cheers
---Dave
> [1] https://lore.kernel.org/patchwork/patch/1314603/
> [2] https://lore.kernel.org/patchwork/cover/1314599/
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
prev parent reply other threads:[~2020-10-05 10:15 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 10:12 [PATCH 0/5] perf: arm64: Support ARMv8.3-SPE extensions Andre Przywara
2020-09-22 10:12 ` [PATCH 1/5] arm64: spe: Allow new bits in SPE filter register Andre Przywara
2020-09-27 2:51 ` Leo Yan
2020-09-22 10:12 ` [PATCH 2/5] perf: arm_spe: Add new event packet bits Andre Przywara
2020-09-27 3:03 ` Leo Yan
2020-09-22 10:12 ` [PATCH 3/5] perf: arm_spe: Add nested virt event decoding Andre Przywara
2020-09-27 3:11 ` Leo Yan
2020-09-22 10:12 ` [PATCH 4/5] perf: arm_spe: Decode memory tagging properties Andre Przywara
2020-09-27 3:19 ` Leo Yan
[not found] ` <20201013145103.GE1063281@kernel.org>
2020-10-13 14:52 ` Arnaldo Carvalho de Melo
2020-09-22 10:12 ` [PATCH 5/5] perf: arm_spe: Decode SVE events Andre Przywara
2020-09-27 3:30 ` Leo Yan
2020-09-28 10:15 ` André Przywara
2020-09-28 11:08 ` Leo Yan
2020-09-28 13:21 ` Dave Martin
2020-09-28 13:59 ` André Przywara
2020-09-28 14:47 ` Dave Martin
2020-09-29 2:19 ` Leo Yan
2020-09-29 14:03 ` Dave Martin
2020-09-30 10:34 ` Dave Martin
2020-09-30 11:04 ` Leo Yan
2020-10-05 10:15 ` Dave Martin [this message]
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=20201005101541.GQ6642@arm.com \
--to=dave.martin@arm.com \
--cc=Al.Grant@arm.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=andre.przywara@arm.com \
--cc=catalin.marinas@arm.com \
--cc=james.clark@arm.com \
--cc=jolsa@redhat.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.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 \
--cc=suzuki.poulose@arm.com \
--cc=tanxiaojun@huawei.com \
--cc=will@kernel.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).