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

[Dave Martin]

On Tue, Sep 29, 2020 at 10:19:02AM +0800, Leo Yan wrote:
> On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> > On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > > On 28/09/2020 14:21, Dave Martin wrote:
> > > 
> > > Hi Dave,
> > > 
> > > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > > >> that introduces very long vector operations (up to 2048 bits).
> > > > 
> > > > (8192, in fact, though don't expect to see that on real hardware any
> > > > time soon...  qemu and the Arm fast model can do it, though.)
> > > > 
> > > >> The SPE profiling feature can tag SVE instructions with additional
> > > >> properties like predication or the effective vector length.
> > > >>
> > > >> Decode the new operation type bits in the SPE decoder to allow the perf
> > > >> tool to correctly report about SVE instructions.
> > > > 
> > > > 
> > > > I don't know anything about SPE, so just commenting on a few minor
> > > > things that catch my eye here.
> > > 
> > > Many thanks for taking a look!
> > > Please note that I actually missed a prior submission by Wei, so the
> > > code changes here will end up in:
> > > https://lore.kernel.org/patchwork/patch/1288413/
> > > 
> > > But your two points below magically apply to his patch as well, so....
> > > 
> > > > 
> > > >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > >> ---
> > > >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> > > >>  1 file changed, 47 insertions(+), 1 deletion(-)
> > > >>
> > > >> 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;

[...]

Best to keep such refactoring independent of this series though.

Cheers
---Dave