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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1054EC3065A for ; Mon, 1 Jul 2024 16:15:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rCrVwpTH06F7iPVCQ6+LQCcNGV+XTGpAtyxINgerc9U=; b=BZmgCfbyG6ArjjLgW7djbgZZB5 NFJmXVlg+Wls/fQ0/q7Be2JriRTxucOK5nus37MSpfkp2h+VmBXulvXJnvSdz7lZHPxyBwnvZH2iG BxFEi88JA3ks0x7J2SP6sbiEQfhp8UrmvVik30iZ5WVaPO5cBr3byF1hVX4Rce7r+sql3/ZPsvkgX ZWWrVoXq1LYEwFFXhwhcZ5xU59XA405s4B+6VuoNL3tM0d7QJodwaLpkaFpzGGrNT78o5qwpXVCMx j4Eo0vgcYEg9lgCno3PfwfqY1c9oLKNiswYy/UvMu8PBg1mf7GGr521KEkZ46r8wlrZbgtIGPPKci 3pZsaT4g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sOJgu-000000044d3-2Ibl; Mon, 01 Jul 2024 16:15:44 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sOJgi-000000044bV-1et1 for linux-arm-kernel@lists.infradead.org; Mon, 01 Jul 2024 16:15:34 +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 09F68339; Mon, 1 Jul 2024 09:15:56 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2F65B3F73B; Mon, 1 Jul 2024 09:15:30 -0700 (PDT) Date: Mon, 1 Jul 2024 17:15:27 +0100 From: Cristian Marussi To: Luke Parkin Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, arm-scmi@vger.kernel.org, sudeep.holla@arm.com, cristian.marussi@arm.com Subject: Re: [PATCH 2/3] Track basic SCMI statistics Message-ID: References: <20240701142851.1448515-1-luke.parkin@arm.com> <20240701142851.1448515-3-luke.parkin@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240701142851.1448515-3-luke.parkin@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240701_091532_570760_B0CD777E X-CRM114-Status: GOOD ( 32.07 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jul 01, 2024 at 03:28:50PM +0100, Luke Parkin wrote: > Add scmi_debug_stats struct with atomic_t types to prevent racing. > Hi "Add SCMI debug statisticts based on atomic types to prevent races." if you want to specify why you did it this way in the commit message, but this seems really more something for a comment in the doxygen that on the commit log. > Add tracking of 5 initial statistics > - sent_ok & sent_fail > - response_ok & dlyd_response_ok > - xfers_response_timeout Morover I would not specify here in the log what you added, you can see it from the code. "Add some initial stats counter" if you want. > > Signed-off-by: Luke Parkin > --- > drivers/firmware/arm_scmi/driver.c | 46 +++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 6b6957f4743f..f69dff699d48 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -125,6 +125,22 @@ struct scmi_debug_info { > bool is_atomic; > }; > > +/** > + * struct scmi_debug_stats - Debug statistics > + * @sent_ok: Count of successful sends > + * @sent_fail: Count of failed sends > + * @response_ok: Count of successful responses > + * @dlyd_response_ok: Count of successful delayed responses > + * @xfers_response_timeout: Count of xfer response timeouts > + */ > +struct scmi_debug_stats { > + atomic_t sent_ok; > + atomic_t sent_fail; > + atomic_t response_ok; > + atomic_t dlyd_response_ok; > + atomic_t xfers_response_timeout; > +}; > + > /** > * struct scmi_info - Structure representing a SCMI instance > * > @@ -141,6 +157,7 @@ struct scmi_debug_info { > * @protocols: IDR for protocols' instance descriptors initialized for > * this SCMI instance: populated on protocol's first attempted > * usage. > + * @stats: Contains several atomic_t's for tracking various statistics > * @protocols_mtx: A mutex to protect protocols instances initialization. > * @protocols_imp: List of protocols implemented, currently maximum of > * scmi_revision_info.num_protocols elements allocated by the > @@ -174,6 +191,7 @@ struct scmi_info { > struct idr tx_idr; > struct idr rx_idr; > struct idr protocols; > + struct scmi_debug_stats stats; Pleaase move this field right after scmi_debug_info down below, so that we keep all debug stuff together. Also this is an embedded structure, not a bare pointer to dynamically allocated stuff (and this is fine...) so you should probably ifdef ARM_SCMI_DEBUG_STATISTICS at compile time the presence of this field itself so as not to waste memory for something you never use...but this is opinable because it will also, on the other side, polllute a bit the code with unpleasant ifdeffery... so maybe Sudeep wont like it...the other option would be to make this a pointer and conditionaly devm_kzalloc a struct to this pointer (like scmi_debug_info) > /* Ensure mutual exclusive access to protocols instance array */ > struct mutex protocols_mtx; > u8 *protocols_imp; > @@ -1143,7 +1161,12 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, > SCMI_RAW_REPLY_QUEUE, > cinfo->id); > } > - > + if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) { > + if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) > + atomic_inc(&info->stats.dlyd_response_ok); > + else > + atomic_inc(&info->stats.response_ok); > + } Ok, so IMO, this is the main core thing to rework in this series: the "counting" operation/block should be defined as a macro so that it can be fully compiled out when STATS=n, because these are counters incremented on the hot path for each message, not just once in a while, so the above if(IS_ENABELD()) now will be there and evaluated even when STATS=n. Something like: #ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS #define SCMI_LOG_STATS(counter) \ \ #else #define SCMI_LOG_STATS(counter) #endif .... I have not thought it through eh...so it could be radically different...the point is ... the counting machinery should just disappear at compile time when STATS=n Moreover please define this macros magic here in this patch BUT split out in a distinct patch all the places where you make use of it, so that this patch contains only definitions of mechanisms and struct and another patch will contain all the places where stats are monitored. > scmi_xfer_command_release(info, xfer); > } > > @@ -1279,6 +1302,12 @@ static int scmi_wait_for_reply(struct device *dev, const struct scmi_desc *desc, > } > } > > + if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS) && ret == -ETIMEDOUT) { > + struct scmi_info *info = > + handle_to_scmi_info(cinfo->handle); > + atomic_inc(&info->stats.xfers_response_timeout); > + } > + Ditto. > return ret; > } > > @@ -1414,6 +1443,13 @@ static int do_xfer(const struct scmi_protocol_handle *ph, > trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id, > xfer->hdr.protocol_id, xfer->hdr.seq, ret); > > + if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) { > + if (ret == 0) > + atomic_inc(&info->stats.sent_ok); > + else > + atomic_inc(&info->stats.sent_fail); > + } > + Ditto. > return ret; > } > > @@ -2994,6 +3030,14 @@ static int scmi_probe(struct platform_device *pdev) > handle->devm_protocol_get = scmi_devm_protocol_get; > handle->devm_protocol_put = scmi_devm_protocol_put; > > + if (IS_ENABLED(CONFIG_ARM_SCMI_DEBUG_STATISTICS)) { > + atomic_set(&info->stats.response_ok, 0); > + atomic_set(&info->stats.sent_fail, 0); > + atomic_set(&info->stats.sent_ok, 0); > + atomic_set(&info->stats.dlyd_response_ok, 0); > + atomic_set(&info->stats.xfers_response_timeout, 0); > + } > + I think that you can just drop this zero initializations because the stats are part of the info structure which is created devm_kzalloc'ed... so zerod at creation time AND indeed atomic_t is just a structure containing an int counter which will be already zeroed too...I dont think that any other special init is done, it is already zero, and nothing more is done by your atomic_set(v, 0)...but feel free to verify I am not missing something, please. Thanks, Cristian