From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 972B7C56202 for ; Thu, 26 Nov 2020 12:13:43 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 080852087C for ; Thu, 26 Nov 2020 12:13:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="MPLZzCP4"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="vlYZf9Nn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 080852087C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=pVmJA4DHf0gGinYTYSsUf0ntK1LdjcsNHWIGGMIcGMM=; b=MPLZzCP4W8jb2U1bNZtABBbFe MX0WmjaZiDZd5+JPpIF9Tzga5ZvoNJW+5iIF4s5y8f3WL1xffArkHCCScNmzF3FR1I9TKDVwYkty0 ioc675wSmrBQ9r5rhTBlRkc9101oltTRVFq2a2tbwlmlhQPJ+Tk5kDVkBb7yEHHySyedg85hWSowj zVp8xF9i35tu1k3JT/bb+SG++zB/HYT2/072sSDiB8Tnu8m1MDG/gqcZt//btbnHMPs0cT3TQYoRX i1jX4xvMDHvLsEtL31P8Pg0A6UtgAJTJhU8yN1qwZHRMk6ODf197CxoaXLzPRBPjDHEyB53vuURth oIS2H14EQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kiG88-0006QE-3f; Thu, 26 Nov 2020 12:12:08 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kiG82-0006PJ-3O for linux-arm-kernel@merlin.infradead.org; Thu, 26 Nov 2020 12:12:05 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=MBVXygyaeVu6g4Ag76aZswt+qb6fgWKDcR5MbwLX7Qk=; b=vlYZf9Nn+51TsNrzuumgsHer/K OW3aWQJRrbHIpS65Kvkje4zhLLZlmOLJXOLVna2JQBsn5bDoSKWjOa7wRrcrh6ogaDfAlBZsVLSPG GroI9BSn+ni1XvIyAJRLn7OeRK6HhdjWRntM+xuKZDioTeW0Bj04qg1l7wTtZNv1qM/RoX29kAzWt yBs3wOaTrLS6PbFb/3HnmOzYxnKDR4/VbuRhGZEKLzrBUObXZ8Wz05H7lLly+jwoBrw+BTqOV1T+v BcoE7Fnh26NxBtHaAtdIFA9VtLUBdEVnHb1gSIJcqpOZYckZC24OlBbK9NrYiJt7lvc3r0+FwUOTW ap2PuIJA==; Received: from [179.97.37.151] (helo=quaco.ghostprotocols.net) by casper.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1kiG80-0000Rl-AX; Thu, 26 Nov 2020 12:12:01 +0000 Received: by quaco.ghostprotocols.net (Postfix, from userid 1000) id 4620540E29; Thu, 26 Nov 2020 09:11:58 -0300 (-03) Date: Thu, 26 Nov 2020 09:11:58 -0300 From: Arnaldo Carvalho de Melo To: Leo Yan Subject: Re: [PATCH v9 01/16] perf arm-spe: Refactor printing string to buffer Message-ID: <20201126121158.GB53384@kernel.org> References: <20201119152441.6972-1-leo.yan@linaro.org> <20201119152441.6972-2-leo.yan@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201119152441.6972-2-leo.yan@linaro.org> X-Url: http://acmel.wordpress.com X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Al Grant , Mathieu Poirier , Will Deacon , Peter Zijlstra , Andre Przywara , John Garry , linux-kernel@vger.kernel.org, Alexander Shishkin , Ingo Molnar , James Clark , Namhyung Kim , Jiri Olsa , Dave Martin , linux-arm-kernel@lists.infradead.org, Wei Li Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Em Thu, Nov 19, 2020 at 11:24:26PM +0800, Leo Yan escreveu: > When outputs strings to the decoding buffer with function snprintf(), > SPE decoder needs to detects if any error returns from snprintf() and if > so needs to directly bail out. If snprintf() returns success, it needs > to update buffer pointer and reduce the buffer length so can continue to > output the next string into the consequent memory space. > > This complex logics are spreading in the function arm_spe_pkt_desc() so > there has many duplicate codes for handling error detecting, increment > buffer pointer and decrement buffer size. > > To avoid the duplicate code, this patch introduces a new helper function > arm_spe_pkt_out_string() which is used to wrap up the complex logics, > and it's used by the caller arm_spe_pkt_desc(). This patch moves the > variable 'blen' as the function's local variable so allows to remove > the unnecessary braces and improve the readability. > > 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_out_string()'s parameter 'err', the 'err' is a > cumulative value, returns its final value if printing buffer is called > for one time or multiple times. Finally, the error is handled 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 > Signed-off-by: Leo Yan > Reviewed-by: Andre Przywara > Reviewed-by: Dave Martin > --- > .../arm-spe-decoder/arm-spe-pkt-decoder.c | 302 +++++++++--------- > tools/perf/util/arm-spe.c | 2 +- > 2 files changed, 151 insertions(+), 153 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 671a4763fb47..fbededc1bcd4 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 > @@ -9,6 +9,7 @@ > #include > #include > #include > +#include > > #include "arm-spe-pkt-decoder.h" > > @@ -258,192 +259,189 @@ int arm_spe_get_packet(const unsigned char *buf, size_t len, > return ret; > } > > +static int arm_spe_pkt_out_string(int *err, char **buf_p, size_t *blen, > + const char *fmt, ...) > +{ > + va_list ap; > + int ret; Ok, from a quick look this continues confusing, but at least its not named scnprintf() and then people won't expect the usual semantics. Applying. - Arnaldo > + /* Bail out if any error occurred */ > + if (err && *err) > + return *err; > + > + va_start(ap, fmt); > + ret = vsnprintf(*buf_p, *blen, fmt, ap); > + va_end(ap); > + > + if (ret < 0) { > + if (err && !*err) > + *err = ret; > + > + /* > + * A return value of *blen or more means that the output was > + * truncated and the buffer is overrun. > + */ > + } else if ((size_t)ret >= *blen) { > + (*buf_p)[*blen - 1] = '\0'; > + > + /* > + * Set *err to 'ret' to avoid overflow if tries to > + * fill this buffer sequentially. > + */ > + if (err && !*err) > + *err = ret; > + } else { > + *buf_p += ret; > + *blen -= ret; > + } > + > + return ret; > +} > + > 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; > > switch (packet->type) { > case ARM_SPE_BAD: > case ARM_SPE_PAD: > case ARM_SPE_END: > - return snprintf(buf, buf_len, "%s", name); > - case ARM_SPE_EVENTS: { > - size_t blen = buf_len; > - > - ret = 0; > - ret = snprintf(buf, buf_len, "EV"); > - buf += ret; > - blen -= ret; > - if (payload & 0x1) { > - ret = snprintf(buf, buf_len, " EXCEPTION-GEN"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x2) { > - ret = snprintf(buf, buf_len, " RETIRED"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x4) { > - ret = snprintf(buf, buf_len, " L1D-ACCESS"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x8) { > - ret = snprintf(buf, buf_len, " L1D-REFILL"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x10) { > - ret = snprintf(buf, buf_len, " TLB-ACCESS"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x20) { > - ret = snprintf(buf, buf_len, " TLB-REFILL"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x40) { > - ret = snprintf(buf, buf_len, " NOT-TAKEN"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x80) { > - ret = snprintf(buf, buf_len, " MISPRED"); > - buf += ret; > - blen -= ret; > - } > + arm_spe_pkt_out_string(&err, &buf, &blen, "%s", name); > + break; > + case ARM_SPE_EVENTS: > + arm_spe_pkt_out_string(&err, &buf, &blen, "EV"); > + > + if (payload & 0x1) > + arm_spe_pkt_out_string(&err, &buf, &blen, " EXCEPTION-GEN"); > + if (payload & 0x2) > + arm_spe_pkt_out_string(&err, &buf, &blen, " RETIRED"); > + if (payload & 0x4) > + arm_spe_pkt_out_string(&err, &buf, &blen, " L1D-ACCESS"); > + if (payload & 0x8) > + arm_spe_pkt_out_string(&err, &buf, &blen, " L1D-REFILL"); > + if (payload & 0x10) > + arm_spe_pkt_out_string(&err, &buf, &blen, " TLB-ACCESS"); > + if (payload & 0x20) > + arm_spe_pkt_out_string(&err, &buf, &blen, " TLB-REFILL"); > + if (payload & 0x40) > + arm_spe_pkt_out_string(&err, &buf, &blen, " NOT-TAKEN"); > + if (payload & 0x80) > + arm_spe_pkt_out_string(&err, &buf, &blen, " MISPRED"); > if (idx > 1) { > - if (payload & 0x100) { > - ret = snprintf(buf, buf_len, " LLC-ACCESS"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x200) { > - ret = snprintf(buf, buf_len, " LLC-REFILL"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x400) { > - ret = snprintf(buf, buf_len, " REMOTE-ACCESS"); > - buf += ret; > - blen -= ret; > - } > + if (payload & 0x100) > + arm_spe_pkt_out_string(&err, &buf, &blen, " LLC-ACCESS"); > + if (payload & 0x200) > + arm_spe_pkt_out_string(&err, &buf, &blen, " LLC-REFILL"); > + if (payload & 0x400) > + arm_spe_pkt_out_string(&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 snprintf(buf, buf_len, "%s", payload & 0x1 ? > - "COND-SELECT" : "INSN-OTHER"); > - case 1: { > - size_t blen = buf_len; > + case 0: > + arm_spe_pkt_out_string(&err, &buf, &blen, > + payload & 0x1 ? "COND-SELECT" : "INSN-OTHER"); > + break; > + case 1: > + arm_spe_pkt_out_string(&err, &buf, &blen, > + payload & 0x1 ? "ST" : "LD"); > > - if (payload & 0x1) > - ret = snprintf(buf, buf_len, "ST"); > - else > - ret = snprintf(buf, buf_len, "LD"); > - buf += ret; > - blen -= ret; > if (payload & 0x2) { > - if (payload & 0x4) { > - ret = snprintf(buf, buf_len, " AT"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x8) { > - ret = snprintf(buf, buf_len, " EXCL"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x10) { > - ret = snprintf(buf, buf_len, " AR"); > - buf += ret; > - blen -= ret; > - } > + if (payload & 0x4) > + arm_spe_pkt_out_string(&err, &buf, &blen, " AT"); > + if (payload & 0x8) > + arm_spe_pkt_out_string(&err, &buf, &blen, " EXCL"); > + if (payload & 0x10) > + arm_spe_pkt_out_string(&err, &buf, &blen, " AR"); > } else if (payload & 0x4) { > - ret = snprintf(buf, buf_len, " SIMD-FP"); > - buf += ret; > - blen -= ret; > - } > - if (ret < 0) > - return ret; > - blen -= ret; > - return buf_len - blen; > - } > - case 2: { > - size_t blen = buf_len; > - > - ret = snprintf(buf, buf_len, "B"); > - buf += ret; > - blen -= ret; > - if (payload & 0x1) { > - ret = snprintf(buf, buf_len, " COND"); > - buf += ret; > - blen -= ret; > - } > - if (payload & 0x2) { > - ret = snprintf(buf, buf_len, " IND"); > - buf += ret; > - blen -= ret; > - } > - if (ret < 0) > - return ret; > - blen -= ret; > - return buf_len - blen; > + arm_spe_pkt_out_string(&err, &buf, &blen, " SIMD-FP"); > } > - default: return 0; > + break; > + case 2: > + arm_spe_pkt_out_string(&err, &buf, &blen, "B"); > + > + if (payload & 0x1) > + arm_spe_pkt_out_string(&err, &buf, &blen, " COND"); > + if (payload & 0x2) > + arm_spe_pkt_out_string(&err, &buf, &blen, " IND"); > + > + break; > + default: > + /* Unknown index */ > + err = -1; > + break; > } > + break; > case ARM_SPE_DATA_SOURCE: > case ARM_SPE_TIMESTAMP: > - return snprintf(buf, buf_len, "%s %lld", name, payload); > + arm_spe_pkt_out_string(&err, &buf, &blen, "%s %lld", name, payload); > + break; > case ARM_SPE_ADDRESS: > switch (idx) { > case 0: > - case 1: ns = !!(packet->payload & NS_FLAG); > + case 1: > + ns = !!(packet->payload & NS_FLAG); > el = (packet->payload & EL_FLAG) >> 61; > payload &= ~(0xffULL << 56); > - return snprintf(buf, buf_len, "%s 0x%llx el%d ns=%d", > + arm_spe_pkt_out_string(&err, &buf, &blen, > + "%s 0x%llx el%d ns=%d", > (idx == 1) ? "TGT" : "PC", payload, el, ns); > - case 2: return snprintf(buf, buf_len, "VA 0x%llx", payload); > - case 3: ns = !!(packet->payload & NS_FLAG); > + break; > + case 2: > + arm_spe_pkt_out_string(&err, &buf, &blen, > + "VA 0x%llx", payload); > + break; > + case 3: > + ns = !!(packet->payload & NS_FLAG); > payload &= ~(0xffULL << 56); > - return snprintf(buf, buf_len, "PA 0x%llx ns=%d", > - payload, ns); > - default: return 0; > + arm_spe_pkt_out_string(&err, &buf, &blen, > + "PA 0x%llx ns=%d", payload, ns); > + break; > + default: > + /* Unknown index */ > + err = -1; > + break; > } > + break; > case ARM_SPE_CONTEXT: > - return snprintf(buf, buf_len, "%s 0x%lx el%d", name, > - (unsigned long)payload, idx + 1); > - case ARM_SPE_COUNTER: { > - size_t blen = buf_len; > - > - ret = snprintf(buf, buf_len, "%s %d ", name, > - (unsigned short)payload); > - buf += ret; > - blen -= ret; > + arm_spe_pkt_out_string(&err, &buf, &blen, "%s 0x%lx el%d", > + name, (unsigned long)payload, idx + 1); > + break; > + case ARM_SPE_COUNTER: > + arm_spe_pkt_out_string(&err, &buf, &blen, "%s %d ", name, > + (unsigned short)payload); > switch (idx) { > - case 0: ret = snprintf(buf, buf_len, "TOT"); break; > - case 1: ret = snprintf(buf, buf_len, "ISSUE"); break; > - case 2: ret = snprintf(buf, buf_len, "XLAT"); break; > - default: ret = 0; > + case 0: > + arm_spe_pkt_out_string(&err, &buf, &blen, "TOT"); > + break; > + case 1: > + arm_spe_pkt_out_string(&err, &buf, &blen, "ISSUE"); > + break; > + case 2: > + arm_spe_pkt_out_string(&err, &buf, &blen, "XLAT"); > + break; > + default: > + break; > } > - if (ret < 0) > - return ret; > - blen -= ret; > - return buf_len - blen; > - } > + break; > default: > + /* Unknown packet type */ > + err = -1; > break; > } > > - return snprintf(buf, buf_len, "%s 0x%llx (%d)", > - name, payload, packet->index); > + /* Output raw data if detect any error */ > + if (err) { > + err = 0; > + arm_spe_pkt_out_string(&err, &buf_orig, &buf_len, "%s 0x%llx (%d)", > + name, payload, packet->index); > + } > + > + return err; > } > 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); > } else { > color_fprintf(stdout, color, " Bad packet!\n"); > -- > 2.17.1 > -- - Arnaldo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel