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 BEA1EC3064D for ; Tue, 2 Jul 2024 11:10:47 +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=MN8LiOLUWQsa2chArmELXTRPk3AiTEZiz/hbfphzk74=; b=Vx1Ky1725zedChQRAyMlnsJOF9 loddqvDrUGzKreKPSpsmBTnmWb/XuZ71TQWdgfVMQNVKGd+R+uPl3vTUJuXeHM6G+wiN6pN5H6wJM 2Sj9DkLhRBBJduGDMUF/uDWKhPef3dfrXBiybp65fMpEquB4cI9DSqHnN8Z4ZoZtRRQ0WsF0w/f05 jFk6yoLdlWLgGuF9jhpkf2tFIsBw7C0RoT8z4JZZv9DYbIXcoZtqHZaRQAHaPXOXKjIFTSLs6sd/T Qk3FY5+CVMxCZfg00+cH+GBHIrMx+BxqtQyrw3kWu9/7QLxx1GajX4ItYIgA6unCMsnP2UR2OJ6E8 xVH+nkhQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sObPB-00000006RXX-3XHZ; Tue, 02 Jul 2024 11:10:37 +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 1sObP0-00000006RWK-2uc1 for linux-arm-kernel@lists.infradead.org; Tue, 02 Jul 2024 11:10:28 +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 72550339; Tue, 2 Jul 2024 04:10:50 -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 7E7013F762; Tue, 2 Jul 2024 04:10:24 -0700 (PDT) Date: Tue, 2 Jul 2024 12:10:21 +0100 From: Cristian Marussi To: Luke Parkin Cc: Cristian Marussi , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "arm-scmi@vger.kernel.org" , Sudeep Holla 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> <490bb053-f2dd-4c6a-a976-c8d21d66eb4c@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <490bb053-f2dd-4c6a-a976-c8d21d66eb4c@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240702_041026_803089_68F82C18 X-CRM114-Status: GOOD ( 23.45 ) 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 Tue, Jul 02, 2024 at 10:57:23AM +0100, Luke Parkin wrote: > > 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 > > Hey Cristian, Unless I've missed something, It looks like IS_ENABLED() does do > what you ask for. > In Documentation/process/coding-style.rst:1168 it reccomends using IS_ENABLED > for conditional compilation over #if and #ifdef, saying that the compiler will > constant-fold the conditional away. Yes indeed, it will be compiled out anyway, forgot that despite having it used myself a few lines below :D .... but from the readability standpoint, given that we will sprinkle this all over the code, wont be much clearer to conditionally define once for all an inline function (like mentioned at the start of that coding-style.rst paragraph) or a macro in a header (like common.h) to wrap the atomic_inc #ifdef CONFIG_ARM_SCMI_DEBUG_STATISTICS static inline void scmi_log_stats(atomic_t *cnt) { atomic_inc(cnt); } #else static inline void scmi_log_stats(atomic_t *cnt) { } #endif and then just call it plainly wherever it needs, knowing that the compiler will equally compile it out all-over when STATS=n. ifdeffery is discouraged in the code flow but it is acceptable to define alternative nop fucntions in a header. Also because in some of the callsite you handle 2 stats with some ifcond (conditional on the IS_ENABLED that is good) and that could be a problem, but those calls can be split and placed alone where that some condition is already check normally as in as an example in scmi_handle_response(): if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) { scmi_clear_channel(info, cinfo); complete(xfer->async_done); + scmi_log_stats(&info->stats.dlyd_response_ok); } else { complete(&xfer->done); + scmi_log_stats(&info->stats.response_ok); } ...what do you think, am I missing something else ? Thanks, Cristian