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=-9.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 A1127C4363A for ; Mon, 5 Oct 2020 10:17:18 +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 4BB312078D for ; Mon, 5 Oct 2020 10:17:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="vqxiof5j" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4BB312078D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com 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=4jiTF3pACZcZ0BoFVd8Tus8M9UZZztrv68fra48aD5U=; b=vqxiof5jmStOA2sXyY6InHdmR B6wB9SfWerKbWwIZZ1k6J0EjgyonV8QTAAqRfDq47/j4NKfEtrauYryjtFX+PrMwKTkxkOMMuw8QG rsThO3lyRQJnERLeoYz45ZdXsQV8JB84+rHfhMplFyWTqY10ZxxkbvTLCylQKFaTNmtF4KcZGz7TT o7/m552VrBD35f3jyGrCVQirOP23z5K6c+VkEbytklR37HumldUhWcX4Or/KIRlMxx5/dpatu+nfr rFKtjTqdGWrsivVH5V4pXHEJ9wY/+YAMCSRu8M40OxDpAgOLY281wjUuOmNJW5UsidfddAbICGyeD uyXfmkIzw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kPNXA-000133-5S; Mon, 05 Oct 2020 10:15:56 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kPNX7-00011e-Bz for linux-arm-kernel@lists.infradead.org; Mon, 05 Oct 2020 10:15:54 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3F90D113E; Mon, 5 Oct 2020 03:15:49 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0E1C83F66B; Mon, 5 Oct 2020 03:15:46 -0700 (PDT) Date: Mon, 5 Oct 2020 11:15:44 +0100 From: Dave Martin To: Leo Yan Subject: Re: [PATCH 5/5] perf: arm_spe: Decode SVE events Message-ID: <20201005101541.GQ6642@arm.com> References: <20200922101225.183554-1-andre.przywara@arm.com> <20200922101225.183554-6-andre.przywara@arm.com> <20200928132114.GF6642@arm.com> <8efd63eb-5ae7-0f9a-6c37-ef5e68af4e6c@arm.com> <20200928144755.GI6642@arm.com> <20200929021902.GA16749@leoy-ThinkPad-X240s> <20200930103409.GP6642@arm.com> <20200930110453.GB9968@leoy-ThinkPad-X240s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200930110453.GB9968@leoy-ThinkPad-X240s> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201005_061553_497187_2027C462 X-CRM114-Status: GOOD ( 39.83 ) 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 , Will Deacon , Suzuki K Poulose , Peter Zijlstra , =?iso-8859-1?Q?Andr=E9?= Przywara , Tan Xiaojun , linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Alexander Shishkin , Ingo Molnar , James Clark , Catalin Marinas , Namhyung Kim , Jiri Olsa , 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 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel