linux-kernel - Re: [PATCH 2/3] Track basic SCMI statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f966bd1a-bbce-4ce0-9a67-c1423cc9cb2f@arm.com>
Date: Tue, 2 Jul 2024 13:50:15 +0100
From: Luke Parkin <luke.parkin@....com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "arm-scmi@...r.kernel.org" <arm-scmi@...r.kernel.org>,
 Sudeep Holla <Sudeep.Holla@....com>
Subject: Re: [PATCH 2/3] Track basic SCMI statistics

> #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
> 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 ?

Ah yeah, that looks better to me. I'll use that.

Thanks!
Luke


On 02/07/2024 12:10, Cristian Marussi wrote:
> 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)			\
>>> 		<your magic here> 			\
>>> 	#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