linux-kernel - Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

Open Source and information security mailing list archives

Message-ID: <20201005101541.GQ6642@arm.com>
Date:   Mon, 5 Oct 2020 11:15:44 +0100
From:   Dave Martin <Dave.Martin@....com>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     Mark Rutland <mark.rutland@....com>, Al Grant <Al.Grant@....com>,
        linux-arm-kernel@...ts.infradead.org,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        André Przywara <andre.przywara@....com>,
        Jiri Olsa <jolsa@...hat.com>, linux-kernel@...r.kernel.org,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        James Clark <james.clark@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Will Deacon <will@...nel.org>,
        Tan Xiaojun <tanxiaojun@...wei.com>,
        Wei Li <liwei391@...wei.com>
Subject: Re: [PATCH 5/5] perf: arm_spe: Decode SVE events

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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives