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

[Luke Parkin <luke.parkin@arm.com>]

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