All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Andre Przywara <Andre.Przywara@arm.com>,
	James Clark <James.Clark@arm.com>,
	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>,
	Al Grant <Al.Grant@arm.com>, Wei Li <liwei391@huawei.com>,
	John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 07/22] perf arm-spe: Consolidate arm_spe_pkt_desc()'s return value
Date: Wed, 11 Nov 2020 16:04:30 +0000	[thread overview]
Message-ID: <20201111160429.GO6882@arm.com> (raw)
In-Reply-To: <20201111071149.815-8-leo.yan@linaro.org>

On Wed, Nov 11, 2020 at 07:11:34AM +0000, Leo Yan wrote:
> arm_spe_pkt_desc() returns the length of consumed the buffer for
> the success case; otherwise, it delivers the return value from
> arm_spe_pkt_snprintf(), and returns the last return value if there have
> multiple calling arm_spe_pkt_snprintf().
> 
> Since arm_spe_pkt_snprintf() has the same semantics with vsnprintf() for
> the return value, and vsnprintf() might return value equals to or bigger
> than the parameter 'size' to indicate the truncation.  Because the
> return value is >= 0 when the string is truncated, this condition will
> be returned up the stack as "success".
> 
> This patch simplifies the return value for arm_spe_pkt_desc(): '0' means
> success and other values mean an error has occurred.  To realize this,
> it relies on arm_spe_pkt_snprintf()'s parameter 'err', the 'err' is a
> cumulative value, returns its final value if printing buffer is called
> for one time or multiple times.
> 
> To unify the error value generation, this patch handles error in a
> central place, rather than directly bailing out in switch-cases,
> it returns error at the end of arm_spe_pkt_desc().
> 
> This patch changes the caller arm_spe_dump() to respect the updated
> return value semantics of arm_spe_pkt_desc().
> 
> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 128 +++++++++---------
>  tools/perf/util/arm-spe.c                     |   2 +-
>  2 files changed, 68 insertions(+), 62 deletions(-)
> 
> 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 1970686f7020..424ff5862aa1 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
> @@ -301,9 +301,10 @@ static int arm_spe_pkt_snprintf(int *err, char **buf_p, size_t *blen,
>  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  		     size_t buf_len)
>  {
> -	int ret, ns, el, idx = packet->index;
> +	int ns, el, idx = packet->index;
>  	unsigned long long payload = packet->payload;
>  	const char *name = arm_spe_pkt_name(packet->type);
> +	char *buf_orig = buf;
>  	size_t blen = buf_len;
>  	int err = 0;
>  
> @@ -311,82 +312,76 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	case ARM_SPE_BAD:
>  	case ARM_SPE_PAD:
>  	case ARM_SPE_END:
> -		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s", name);
> +		break;
>  	case ARM_SPE_EVENTS:
> -		ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "EV");
>  
>  		if (payload & 0x1)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCEPTION-GEN");
>  		if (payload & 0x2)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " RETIRED");
>  		if (payload & 0x4)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-ACCESS");
>  		if (payload & 0x8)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " L1D-REFILL");
>  		if (payload & 0x10)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-ACCESS");
>  		if (payload & 0x20)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " TLB-REFILL");
>  		if (payload & 0x40)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " NOT-TAKEN");
>  		if (payload & 0x80)
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, " MISPRED");
>  		if (idx > 1) {
>  			if (payload & 0x100)
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-ACCESS");
>  			if (payload & 0x200)
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " LLC-REFILL");
>  			if (payload & 0x400)
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " REMOTE-ACCESS");
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;
> +		break;
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
>  		case 0:
> -			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
>  					payload & 0x1 ? "COND-SELECT" : "INSN-OTHER");
> +			break;
>  		case 1:
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen,
> -						   payload & 0x1 ? "ST" : "LD");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					     payload & 0x1 ? "ST" : "LD");
>  
>  			if (payload & 0x2) {
>  				if (payload & 0x4)
> -					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AT");
>  				if (payload & 0x8)
> -					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " EXCL");
>  				if (payload & 0x10)
> -					ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
> +					arm_spe_pkt_snprintf(&err, &buf, &blen, " AR");
>  			} else if (payload & 0x4) {
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " SIMD-FP");
>  			}
> -
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> -
> +			break;
>  		case 2:
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "B");
>  
>  			if (payload & 0x1)
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " COND");
>  			if (payload & 0x2)
> -				ret = arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
> -
> -			if (ret < 0)
> -				return ret;
> -			blen -= ret;
> -			return buf_len - blen;
> +				arm_spe_pkt_snprintf(&err, &buf, &blen, " IND");
>  
> +			break;
>  		default:
> -			return 0;
> +			/* Unknown index */
> +			err = -1;
> +			break;
>  		}
> +		break;
>  	case ARM_SPE_DATA_SOURCE:
>  	case ARM_SPE_TIMESTAMP:
> -		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %lld", name, payload);
> +		break;
>  	case ARM_SPE_ADDRESS:
>  		switch (idx) {
>  		case 0:
> @@ -394,48 +389,59 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  			ns = !!(packet->payload & NS_FLAG);
>  			el = (packet->payload & EL_FLAG) >> 61;
>  			payload &= ~(0xffULL << 56);
> -			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
>  					"%s 0x%llx el%d ns=%d",
>  				        (idx == 1) ? "TGT" : "PC", payload, el, ns);
> +			break;
>  		case 2:
> -			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> -						    "VA 0x%llx", payload);
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					     "VA 0x%llx", payload);
> +			break;
>  		case 3:
>  			ns = !!(packet->payload & NS_FLAG);
>  			payload &= ~(0xffULL << 56);
> -			return arm_spe_pkt_snprintf(&err, &buf, &blen,
> -						    "PA 0x%llx ns=%d", payload, ns);
> +			arm_spe_pkt_snprintf(&err, &buf, &blen,
> +					     "PA 0x%llx ns=%d", payload, ns);
> +			break;
>  		default:
> -			return 0;
> +			/* Unknown index */
> +			err = -1;
> +			break;
>  		}
> +		break;
>  	case ARM_SPE_CONTEXT:
> -		return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> -					    name, (unsigned long)payload, idx + 1);
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%lx el%d",
> +				     name, (unsigned long)payload, idx + 1);
> +		break;
>  	case ARM_SPE_COUNTER:
> -		ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> -					   (unsigned short)payload);
> +		arm_spe_pkt_snprintf(&err, &buf, &blen, "%s %d ", name,
> +				     (unsigned short)payload);
>  		switch (idx) {
>  		case 0:
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "TOT");
>  			break;
>  		case 1:
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "ISSUE");
>  			break;
>  		case 2:
> -			ret = arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
> +			arm_spe_pkt_snprintf(&err, &buf, &blen, "XLAT");
>  			break;
>  		default:
> -			ret = 0;
>  			break;
>  		}
> -		if (ret < 0)
> -			return ret;
> -		blen -= ret;
> -		return buf_len - blen;
> +		break;
>  	default:
> +		/* Unknown index */
> +		err = -1;
>  		break;
>  	}
>  
> -	return arm_spe_pkt_snprintf(&err, &buf, &blen, "%s 0x%llx (%d)",
> -				    name, payload, packet->index);
> +	/* Output raw data if detect any error */
> +	if (err) {
> +		err = 0;
> +		arm_spe_pkt_snprintf(&err, &buf_orig, &buf_len, "%s 0x%llx (%d)",
> +				     name, payload, packet->index);
> +	}
> +
> +	return err;

Ah, I understand what this is doing now.  Thanks for the extra comment.

>  }
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 3882a5360ada..8901a1656a41 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -113,7 +113,7 @@ static void arm_spe_dump(struct arm_spe *spe __maybe_unused,
>  		if (ret > 0) {
>  			ret = arm_spe_pkt_desc(&packet, desc,
>  					       ARM_SPE_PKT_DESC_MAX);
> -			if (ret > 0)
> +			if (!ret)
>  				color_fprintf(stdout, color, " %s\n", desc);

The effect of this patch looks OK to me, so

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

[...]

Cheers
---Dave

  reply	other threads:[~2020-11-11 16:04 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-11  7:11 [PATCH v8 00/22] perf arm-spe: Refactor decoding & dumping flow Leo Yan
2020-11-11  7:11 ` [PATCH v8 01/22] perf arm-spe: Include bitops.h for BIT() macro Leo Yan
2020-11-11  7:11 ` [PATCH v8 02/22] perf arm-spe: Fix a typo in comment Leo Yan
2020-11-11  7:11 ` [PATCH v8 03/22] perf arm-spe: Refactor payload size calculation Leo Yan
2020-11-11  7:11 ` [PATCH v8 04/22] perf arm-spe: Refactor arm_spe_get_events() Leo Yan
2020-11-11  7:11 ` [PATCH v8 05/22] perf arm-spe: Fix packet length handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 06/22] perf arm-spe: Refactor printing string to buffer Leo Yan
2020-11-11 15:35   ` Arnaldo Carvalho de Melo
2020-11-11 15:45     ` André Przywara
2020-11-11 17:39       ` Arnaldo Carvalho de Melo
2020-11-11 17:58         ` Dave Martin
2020-11-11 18:01           ` Arnaldo Carvalho de Melo
2020-11-11 18:02             ` Arnaldo Carvalho de Melo
2020-11-11 18:08               ` Arnaldo Carvalho de Melo
2020-11-12  2:20             ` Leo Yan
2020-11-11 23:03           ` David Laight
2020-11-12  3:02             ` Leo Yan
2020-11-11 15:53   ` Dave Martin
2020-11-11 15:58     ` Dave Martin
2020-11-11 17:40       ` Arnaldo Carvalho de Melo
2020-11-11  7:11 ` [PATCH v8 07/22] perf arm-spe: Consolidate arm_spe_pkt_desc()'s return value Leo Yan
2020-11-11 16:04   ` Dave Martin [this message]
2020-11-11  7:11 ` [PATCH v8 08/22] perf arm-spe: Refactor packet header parsing Leo Yan
2020-11-11  7:11 ` [PATCH v8 09/22] perf arm-spe: Add new function arm_spe_pkt_desc_addr() Leo Yan
2020-11-11  7:11 ` [PATCH v8 10/22] perf arm-spe: Refactor address packet handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 11/22] perf arm_spe: Fixup top byte for data virtual address Leo Yan
2020-11-11  7:11 ` [PATCH v8 12/22] perf arm-spe: Refactor context packet handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 13/22] perf arm-spe: Add new function arm_spe_pkt_desc_counter() Leo Yan
2020-11-11  7:11 ` [PATCH v8 14/22] perf arm-spe: Refactor counter packet handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 15/22] perf arm-spe: Add new function arm_spe_pkt_desc_event() Leo Yan
2020-11-11  7:11 ` [PATCH v8 16/22] perf arm-spe: Refactor event type handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 17/22] perf arm-spe: Remove size condition checking for events Leo Yan
2020-11-11  7:11 ` [PATCH v8 18/22] perf arm-spe: Add new function arm_spe_pkt_desc_op_type() Leo Yan
2020-11-11  7:11 ` [PATCH v8 19/22] perf arm-spe: Refactor operation packet handling Leo Yan
2020-11-11  7:11 ` [PATCH v8 20/22] perf arm-spe: Add more sub classes for operation packet Leo Yan
2020-11-11  7:11 ` [PATCH v8 21/22] perf arm_spe: Decode memory tagging properties Leo Yan
2020-11-11  7:11 ` [PATCH v8 22/22] perf arm-spe: Add support for ARMv8.3-SPE Leo Yan
2020-11-11 10:13 ` [PATCH v8 00/22] perf arm-spe: Refactor decoding & dumping flow André Przywara
2020-11-11 11:58   ` Arnaldo Carvalho de Melo
2020-11-11 16:10 ` Arnaldo Carvalho de Melo
2020-11-11 16:15   ` Arnaldo Carvalho de Melo
2020-11-11 16:20     ` André Przywara
2020-11-11 17:44       ` Arnaldo Carvalho de Melo
2020-11-11 17:51         ` André Przywara

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=20201111160429.GO6882@arm.com \
    --to=dave.martin@arm.com \
    --cc=Al.Grant@arm.com \
    --cc=Andre.Przywara@arm.com \
    --cc=James.Clark@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=john.garry@huawei.com \
    --cc=jolsa@redhat.com \
    --cc=leo.yan@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liwei391@huawei.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.