Re: [PATCH net-next 2/2] net/smc: introduce statistics for ringbufs usage of net namespace

[Simon Horman <horms@kernel.org>]

On Mon, Aug 05, 2024 at 05:05:51PM +0800, Wen Gu wrote:
> The buffer size histograms in smc_stats, namely rx/tx_rmbsize, record
> the sizes of ringbufs for all connections that have ever appeared in
> the net namespace. They are incremental and we cannot know the actual
> ringbufs usage from these. So here introduces statistics for current
> ringbufs usage of existing smc connections in the net namespace into
> smc_stats, it will be incremented when new connection uses a ringbuf
> and decremented when the ringbuf is unused.
> 
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>

...

> diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h

...

> @@ -135,38 +137,45 @@ do { \
>  } \
>  while (0)
>  
> -#define SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _len) \
> +#define SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _is_add, _len) \
>  do { \
> +	typeof(_is_add) is_a = (_is_add); \
>  	typeof(_len) _l = (_len); \
>  	typeof(_tech) t = (_tech); \
>  	int _pos; \
>  	int m = SMC_BUF_MAX - 1; \
>  	if (_l <= 0) \
>  		break; \
> -	_pos = fls((_l - 1) >> 13); \
> -	_pos = (_pos <= m) ? _pos : m; \
> -	this_cpu_inc((*(_smc_stats)).smc[t].k ## _rmbsize.buf[_pos]); \
> +	if (is_a) { \
> +		_pos = fls((_l - 1) >> 13); \
> +		_pos = (_pos <= m) ? _pos : m; \
> +		this_cpu_inc((*(_smc_stats)).smc[t].k ## _rmbsize.buf[_pos]); \
> +		this_cpu_add((*(_smc_stats)).smc[t].k ## _rmbuse, _l); \

Nit:

I see that due to the construction of the caller, SMC_STAT_RMB_SIZE(),
it will not occur. But checkpatch warns of possible side effects
from reuse of _smc_stats.

As great care seems to have been taken in these macros to avoid such
problems, even if theoretical, perhaps it is worth doing so here too.

f.e. A macro-local variable could store (*(_smc_stats)).smc[t] which
     I think would both resolve the problem mentioned, and make some
     lines shorter (and maybe easier to read).

> +	} else { \
> +		this_cpu_sub((*(_smc_stats)).smc[t].k ## _rmbuse, _l); \
> +	} \
>  } \
>  while (0)
>  
>  #define SMC_STAT_RMB_SUB(_smc_stats, type, t, key) \
>  	this_cpu_inc((*(_smc_stats)).smc[t].rmb ## _ ## key.type ## _cnt)
>  
> -#define SMC_STAT_RMB_SIZE(_smc, _is_smcd, _is_rx, _len) \
> +#define SMC_STAT_RMB_SIZE(_smc, _is_smcd, _is_rx, _is_add, _len) \
>  do { \
>  	struct net *_net = sock_net(&(_smc)->sk); \
>  	struct smc_stats __percpu *_smc_stats = _net->smc.smc_stats; \
> +	typeof(_is_add) is_add = (_is_add); \
>  	typeof(_is_smcd) is_d = (_is_smcd); \
>  	typeof(_is_rx) is_r = (_is_rx); \
>  	typeof(_len) l = (_len); \
>  	if ((is_d) && (is_r)) \
> -		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, rx, l); \
> +		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, rx, is_add, l); \
>  	if ((is_d) && !(is_r)) \
> -		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, tx, l); \
> +		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, tx, is_add, l); \
>  	if (!(is_d) && (is_r)) \
> -		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, rx, l); \
> +		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, rx, is_add, l); \
>  	if (!(is_d) && !(is_r)) \
> -		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, tx, l); \
> +		SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, tx, is_add, l); \
>  } \
>  while (0)
>  
> -- 
> 2.32.0.3.g01195cf9f
> 
>