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

[Cristian Marussi <cristian.marussi@arm.com>]

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