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

Simon Horman
2024-08-06T10:49:29Z

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