Re: [PATCH net-next 1/2] net/smc: introduce statistics for allocated ringbufs of link group

[Simon Horman <horms@kernel.org>]

On Mon, Aug 05, 2024 at 05:05:50PM +0800, Wen Gu wrote:
> Currently we have the statistics on sndbuf/RMB sizes of all connections
> that have ever been on the link group, namely smc_stats_memsize. However
> these statistics are incremental and since the ringbufs of link group
> are allowed to be reused, we cannot know the actual allocated buffers
> through these. So here introduces the statistic on actual allocated
> ringbufs of the link group, it will be incremented when a new ringbuf is
> added into buf_list and decremented when it is deleted from buf_list.
> 
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
>  include/uapi/linux/smc.h |  4 ++++
>  net/smc/smc_core.c       | 52 ++++++++++++++++++++++++++++++++++++----
>  net/smc/smc_core.h       |  2 ++
>  3 files changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/smc.h b/include/uapi/linux/smc.h
> index b531e3ef011a..d27b8dc50f90 100644
> --- a/include/uapi/linux/smc.h
> +++ b/include/uapi/linux/smc.h
> @@ -127,6 +127,8 @@ enum {
>  	SMC_NLA_LGR_R_NET_COOKIE,	/* u64 */
>  	SMC_NLA_LGR_R_PAD,		/* flag */
>  	SMC_NLA_LGR_R_BUF_TYPE,		/* u8 */
> +	SMC_NLA_LGR_R_SNDBUF_ALLOC,	/* u64 */
> +	SMC_NLA_LGR_R_RMB_ALLOC,	/* u64 */
>  	__SMC_NLA_LGR_R_MAX,
>  	SMC_NLA_LGR_R_MAX = __SMC_NLA_LGR_R_MAX - 1
>  };
> @@ -162,6 +164,8 @@ enum {
>  	SMC_NLA_LGR_D_V2_COMMON,	/* nest */
>  	SMC_NLA_LGR_D_EXT_GID,		/* u64 */
>  	SMC_NLA_LGR_D_PEER_EXT_GID,	/* u64 */
> +	SMC_NLA_LGR_D_SNDBUF_ALLOC,	/* u64 */
> +	SMC_NLA_LGR_D_DMB_ALLOC,	/* u64 */
>  	__SMC_NLA_LGR_D_MAX,
>  	SMC_NLA_LGR_D_MAX = __SMC_NLA_LGR_D_MAX - 1
>  };
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 71fb334d8234..73c7999fc74f 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -221,6 +221,37 @@ static void smc_lgr_unregister_conn(struct smc_connection *conn)
>  	write_unlock_bh(&lgr->conns_lock);
>  }
>  
> +/* must be called under lgr->{sndbufs|rmbs} lock */
> +static inline void smc_lgr_buf_list_add(struct smc_link_group *lgr,
> +					bool is_rmb,
> +					struct list_head *buf_list,
> +					struct smc_buf_desc *buf_desc)

Please do not use the inline keyword in .c files unless there is a
demonstrable reason to do so, e.g. performance. Rather, please allow
the compiler to inline functions as it sees fit.

The inline keyword in .h files is, of course, fine.

> +{
> +	list_add(&buf_desc->list, buf_list);
> +	if (is_rmb) {
> +		lgr->alloc_rmbs += buf_desc->len;
> +		lgr->alloc_rmbs +=
> +			lgr->is_smcd ? sizeof(struct smcd_cdc_msg) : 0;
> +	} else {
> +		lgr->alloc_sndbufs += buf_desc->len;
> +	}
> +}
> +
> +/* must be called under lgr->{sndbufs|rmbs} lock */
> +static inline void smc_lgr_buf_list_del(struct smc_link_group *lgr,
> +					bool is_rmb,
> +					struct smc_buf_desc *buf_desc)

Ditto.

> +{
> +	list_del(&buf_desc->list);
> +	if (is_rmb) {
> +		lgr->alloc_rmbs -= buf_desc->len;
> +		lgr->alloc_rmbs -=
> +			lgr->is_smcd ? sizeof(struct smcd_cdc_msg) : 0;
> +	} else {
> +		lgr->alloc_sndbufs -= buf_desc->len;
> +	}
> +}
> +

...

-- 
pw-bot: changes-requested