[PATCH net-next 0/2] net/smc: introduce ringbufs usage statistics

[PATCH net-next 0/2] net/smc: introduce ringbufs usage statistics

[Wen Gu]

Currently, we have histograms that show the sizes of ringbufs that ever
used by SMC connections. However, they are always incremental and since
SMC allows the reuse of ringbufs, we cannot know the actual amount of
ringbufs being allocated or actively used.

So this patch set introduces statistics for the amount of ringbufs that
actually allocated by link group and actively used by connections of a
certain net namespace, so that we can react based on these memory usage
information, e.g. active fallback to TCP.

With appropriate adaptations of smc-tools, we can obtain these ringbufs
usage information:

$ smcr -d linkgroup
LG-ID    : 00000500
LG-Role  : SERV
LG-Type  : ASYML
VLAN     : 0
PNET-ID  :
Version  : 1
Conns    : 0
Sndbuf   : 12910592 B    <-
RMB      : 12910592 B    <-

or

$ smcr -d stats
[...]
RX Stats
  Data transmitted (Bytes)      869225943 (869.2M)
  Total requests                 18494479
  Buffer usage  (Bytes)          12910592 (12.31M)  <-
  [...]

TX Stats
  Data transmitted (Bytes)    12760884405 (12.76G)
  Total requests                 36988338
  Buffer usage  (Bytes)          12910592 (12.31M)  <-
  [...]
[...]

Wen Gu (2):
  net/smc: introduce statistics for allocated ringbufs of link group
  net/smc: introduce statistics for ringbufs usage of net namespace

 include/uapi/linux/smc.h |  6 ++++
 net/smc/smc_core.c       | 74 ++++++++++++++++++++++++++++++++++------
 net/smc/smc_core.h       |  2 ++
 net/smc/smc_stats.c      |  8 +++++
 net/smc/smc_stats.h      | 27 ++++++++++-----
 5 files changed, 97 insertions(+), 20 deletions(-)

-- 
2.32.0.3.g01195cf9f

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

[Wen Gu]

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)
+{
+	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)
+{
+	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;
+	}
+}
+
 int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct smc_nl_dmp_ctx *cb_ctx = smc_nl_dmp_ctx(cb);
@@ -363,6 +394,12 @@ static int smc_nl_fill_lgr(struct smc_link_group *lgr,
 	smc_target[SMC_MAX_PNETID_LEN] = 0;
 	if (nla_put_string(skb, SMC_NLA_LGR_R_PNETID, smc_target))
 		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_R_SNDBUF_ALLOC,
+			      lgr->alloc_sndbufs, SMC_NLA_LGR_R_PAD))
+		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_R_RMB_ALLOC,
+			      lgr->alloc_rmbs, SMC_NLA_LGR_R_PAD))
+		goto errattr;
 	if (lgr->smc_version > SMC_V1) {
 		v2_attrs = nla_nest_start(skb, SMC_NLA_LGR_R_V2_COMMON);
 		if (!v2_attrs)
@@ -541,6 +578,12 @@ static int smc_nl_fill_smcd_lgr(struct smc_link_group *lgr,
 		goto errattr;
 	if (nla_put_u32(skb, SMC_NLA_LGR_D_CHID, smc_ism_get_chid(lgr->smcd)))
 		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_SNDBUF_ALLOC,
+			      lgr->alloc_sndbufs, SMC_NLA_LGR_D_PAD))
+		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_DMB_ALLOC,
+			      lgr->alloc_rmbs, SMC_NLA_LGR_D_PAD))
+		goto errattr;
 	memcpy(smc_pnet, lgr->smcd->pnetid, SMC_MAX_PNETID_LEN);
 	smc_pnet[SMC_MAX_PNETID_LEN] = 0;
 	if (nla_put_string(skb, SMC_NLA_LGR_D_PNETID, smc_pnet))
@@ -1138,7 +1181,7 @@ static void smcr_buf_unuse(struct smc_buf_desc *buf_desc, bool is_rmb,
 		lock = is_rmb ? &lgr->rmbs_lock :
 				&lgr->sndbufs_lock;
 		down_write(lock);
-		list_del(&buf_desc->list);
+		smc_lgr_buf_list_del(lgr, is_rmb, buf_desc);
 		up_write(lock);
 
 		smc_buf_free(lgr, is_rmb, buf_desc);
@@ -1377,7 +1420,7 @@ static void __smc_lgr_free_bufs(struct smc_link_group *lgr, bool is_rmb)
 			buf_list = &lgr->sndbufs[i];
 		list_for_each_entry_safe(buf_desc, bf_desc, buf_list,
 					 list) {
-			list_del(&buf_desc->list);
+			smc_lgr_buf_list_del(lgr, is_rmb, buf_desc);
 			smc_buf_free(lgr, is_rmb, buf_desc);
 		}
 	}
@@ -2414,7 +2457,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 		SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, bufsize);
 		buf_desc->used = 1;
 		down_write(lock);
-		list_add(&buf_desc->list, buf_list);
+		smc_lgr_buf_list_add(lgr, is_rmb, buf_list, buf_desc);
 		up_write(lock);
 		break; /* found */
 	}
@@ -2496,7 +2539,8 @@ int smc_buf_create(struct smc_sock *smc, bool is_smcd)
 	rc = __smc_buf_create(smc, is_smcd, true);
 	if (rc && smc->conn.sndbuf_desc) {
 		down_write(&smc->conn.lgr->sndbufs_lock);
-		list_del(&smc->conn.sndbuf_desc->list);
+		smc_lgr_buf_list_del(smc->conn.lgr, false,
+				     smc->conn.sndbuf_desc);
 		up_write(&smc->conn.lgr->sndbufs_lock);
 		smc_buf_free(smc->conn.lgr, false, smc->conn.sndbuf_desc);
 		smc->conn.sndbuf_desc = NULL;
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index d93cf51dbd7c..0db4e5f79ac4 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -281,6 +281,8 @@ struct smc_link_group {
 	struct rw_semaphore	sndbufs_lock;	/* protects tx buffers */
 	struct list_head	rmbs[SMC_RMBE_SIZES];	/* rx buffers */
 	struct rw_semaphore	rmbs_lock;	/* protects rx buffers */
+	u64			alloc_sndbufs;	/* stats of tx buffers */
+	u64			alloc_rmbs;	/* stats of rx buffers */
 
 	u8			id[SMC_LGR_ID_SIZE];	/* unique lgr id */
 	struct delayed_work	free_work;	/* delayed freeing of an lgr */
-- 
2.32.0.3.g01195cf9f

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

[Wen Gu]

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>
---
 include/uapi/linux/smc.h |  2 ++
 net/smc/smc_core.c       | 22 +++++++++++++++-------
 net/smc/smc_stats.c      |  8 ++++++++
 net/smc/smc_stats.h      | 27 ++++++++++++++++++---------
 4 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/include/uapi/linux/smc.h b/include/uapi/linux/smc.h
index d27b8dc50f90..304e202c03bb 100644
--- a/include/uapi/linux/smc.h
+++ b/include/uapi/linux/smc.h
@@ -253,6 +253,8 @@ enum {
 	SMC_NLA_STATS_T_TX_BYTES,	/* u64 */
 	SMC_NLA_STATS_T_RX_CNT,		/* u64 */
 	SMC_NLA_STATS_T_TX_CNT,		/* u64 */
+	SMC_NLA_STATS_T_RX_RMB_USAGE,	/* u64 */
+	SMC_NLA_STATS_T_TX_RMB_USAGE,	/* u64 */
 	__SMC_NLA_STATS_T_MAX,
 	SMC_NLA_STATS_T_MAX = __SMC_NLA_STATS_T_MAX - 1
 };
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 73c7999fc74f..f0e08ed9b434 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1209,22 +1209,30 @@ static void smcd_buf_detach(struct smc_connection *conn)
 static void smc_buf_unuse(struct smc_connection *conn,
 			  struct smc_link_group *lgr)
 {
+	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
+	bool is_smcd = lgr->is_smcd;
+	int bufsize;
+
 	if (conn->sndbuf_desc) {
-		if (!lgr->is_smcd && conn->sndbuf_desc->is_vm) {
+		bufsize = conn->sndbuf_desc->len;
+		if (!is_smcd && conn->sndbuf_desc->is_vm) {
 			smcr_buf_unuse(conn->sndbuf_desc, false, lgr);
 		} else {
-			memzero_explicit(conn->sndbuf_desc->cpu_addr, conn->sndbuf_desc->len);
+			memzero_explicit(conn->sndbuf_desc->cpu_addr, bufsize);
 			WRITE_ONCE(conn->sndbuf_desc->used, 0);
 		}
+		SMC_STAT_RMB_SIZE(smc, is_smcd, false, false, bufsize);
 	}
 	if (conn->rmb_desc) {
-		if (!lgr->is_smcd) {
+		bufsize = conn->rmb_desc->len;
+		if (!is_smcd) {
 			smcr_buf_unuse(conn->rmb_desc, true, lgr);
 		} else {
-			memzero_explicit(conn->rmb_desc->cpu_addr,
-					 conn->rmb_desc->len + sizeof(struct smcd_cdc_msg));
+			bufsize += sizeof(struct smcd_cdc_msg);
+			memzero_explicit(conn->rmb_desc->cpu_addr, bufsize);
 			WRITE_ONCE(conn->rmb_desc->used, 0);
 		}
+		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
 	}
 }
 
@@ -2433,7 +2441,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 		buf_desc = smc_buf_get_slot(bufsize_comp, lock, buf_list);
 		if (buf_desc) {
 			buf_desc->is_dma_need_sync = 0;
-			SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, bufsize);
+			SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, true, bufsize);
 			SMC_STAT_BUF_REUSE(smc, is_smcd, is_rmb);
 			break; /* found reusable slot */
 		}
@@ -2454,7 +2462,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 		}
 
 		SMC_STAT_RMB_ALLOC(smc, is_smcd, is_rmb);
-		SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, bufsize);
+		SMC_STAT_RMB_SIZE(smc, is_smcd, is_rmb, true, bufsize);
 		buf_desc->used = 1;
 		down_write(lock);
 		smc_lgr_buf_list_add(lgr, is_rmb, buf_list, buf_desc);
diff --git a/net/smc/smc_stats.c b/net/smc/smc_stats.c
index ca14c0f3a07d..3f2ebc6c06ba 100644
--- a/net/smc/smc_stats.c
+++ b/net/smc/smc_stats.c
@@ -218,6 +218,14 @@ static int smc_nl_fill_stats_tech_data(struct sk_buff *skb,
 			      smc_tech->tx_bytes,
 			      SMC_NLA_STATS_PAD))
 		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_STATS_T_RX_RMB_USAGE,
+			      smc_tech->rx_rmbuse,
+			      SMC_NLA_STATS_PAD))
+		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_STATS_T_TX_RMB_USAGE,
+			      smc_tech->tx_rmbuse,
+			      SMC_NLA_STATS_PAD))
+		goto errattr;
 	if (nla_put_u64_64bit(skb, SMC_NLA_STATS_T_RX_CNT,
 			      smc_tech->rx_cnt,
 			      SMC_NLA_STATS_PAD))
diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
index 9d32058db2b5..04110ed359b8 100644
--- a/net/smc/smc_stats.h
+++ b/net/smc/smc_stats.h
@@ -79,6 +79,8 @@ struct smc_stats_tech {
 	u64			tx_bytes;
 	u64			rx_cnt;
 	u64			tx_cnt;
+	u64			rx_rmbuse;
+	u64			tx_rmbuse;
 };
 
 struct smc_stats {
@@ -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); \
+	} 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

Re: [PATCH net-next 0/2] net/smc: introduce ringbufs usage statistics

[shaozhengchao]

Hi Wen Gu:
    Your patchset looks fine. However, the current smc-tools tool is not
supported, so will you update the smc-tools tool?

Thank you

Zhengchao Shao

On 2024/8/5 17:05, Wen Gu wrote:
> Currently, we have histograms that show the sizes of ringbufs that ever
> used by SMC connections. However, they are always incremental and since
> SMC allows the reuse of ringbufs, we cannot know the actual amount of
> ringbufs being allocated or actively used.
> 
> So this patch set introduces statistics for the amount of ringbufs that
> actually allocated by link group and actively used by connections of a
> certain net namespace, so that we can react based on these memory usage
> information, e.g. active fallback to TCP.
> 
> With appropriate adaptations of smc-tools, we can obtain these ringbufs
> usage information:
> 
> $ smcr -d linkgroup
> LG-ID    : 00000500
> LG-Role  : SERV
> LG-Type  : ASYML
> VLAN     : 0
> PNET-ID  :
> Version  : 1
> Conns    : 0
> Sndbuf   : 12910592 B    <-
> RMB      : 12910592 B    <-
> 
> or
> 
> $ smcr -d stats
> [...]
> RX Stats
>    Data transmitted (Bytes)      869225943 (869.2M)
>    Total requests                 18494479
>    Buffer usage  (Bytes)          12910592 (12.31M)  <-
>    [...]
> 
> TX Stats
>    Data transmitted (Bytes)    12760884405 (12.76G)
>    Total requests                 36988338
>    Buffer usage  (Bytes)          12910592 (12.31M)  <-
>    [...]
> [...]
> 
> Wen Gu (2):
>    net/smc: introduce statistics for allocated ringbufs of link group
>    net/smc: introduce statistics for ringbufs usage of net namespace
> 
>   include/uapi/linux/smc.h |  6 ++++
>   net/smc/smc_core.c       | 74 ++++++++++++++++++++++++++++++++++------
>   net/smc/smc_core.h       |  2 ++
>   net/smc/smc_stats.c      |  8 +++++
>   net/smc/smc_stats.h      | 27 ++++++++++-----
>   5 files changed, 97 insertions(+), 20 deletions(-)
> 

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

[Simon Horman]

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

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

[Simon Horman]

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

Re: [PATCH net-next 0/2] net/smc: introduce ringbufs usage statistics

[Wen Gu]

On 2024/8/6 11:52, shaozhengchao wrote:
> Hi Wen Gu:
>     Your patchset looks fine. However, the current smc-tools tool is not
> supported, so will you update the smc-tools tool?
> 
> Thank you
> 
> Zhengchao Shao
> 

Hi, Zhengchao.

Yes, after these kernel patches are merged, I will submit the corresponding
modification to smc-tools.

Thanks!

> On 2024/8/5 17:05, Wen Gu wrote:
>> Currently, we have histograms that show the sizes of ringbufs that ever
>> used by SMC connections. However, they are always incremental and since
>> SMC allows the reuse of ringbufs, we cannot know the actual amount of
>> ringbufs being allocated or actively used.
>>
>> So this patch set introduces statistics for the amount of ringbufs that
>> actually allocated by link group and actively used by connections of a
>> certain net namespace, so that we can react based on these memory usage
>> information, e.g. active fallback to TCP.
>>
>> With appropriate adaptations of smc-tools, we can obtain these ringbufs
>> usage information:
>>
>> $ smcr -d linkgroup
>> LG-ID    : 00000500
>> LG-Role  : SERV
>> LG-Type  : ASYML
>> VLAN     : 0
>> PNET-ID  :
>> Version  : 1
>> Conns    : 0
>> Sndbuf   : 12910592 B    <-
>> RMB      : 12910592 B    <-
>>
>> or
>>
>> $ smcr -d stats
>> [...]
>> RX Stats
>>    Data transmitted (Bytes)      869225943 (869.2M)
>>    Total requests                 18494479
>>    Buffer usage  (Bytes)          12910592 (12.31M)  <-
>>    [...]
>>
>> TX Stats
>>    Data transmitted (Bytes)    12760884405 (12.76G)
>>    Total requests                 36988338
>>    Buffer usage  (Bytes)          12910592 (12.31M)  <-
>>    [...]
>> [...]
>>
>> Wen Gu (2):
>>    net/smc: introduce statistics for allocated ringbufs of link group
>>    net/smc: introduce statistics for ringbufs usage of net namespace
>>
>>   include/uapi/linux/smc.h |  6 ++++
>>   net/smc/smc_core.c       | 74 ++++++++++++++++++++++++++++++++++------
>>   net/smc/smc_core.h       |  2 ++
>>   net/smc/smc_stats.c      |  8 +++++
>>   net/smc/smc_stats.h      | 27 ++++++++++-----
>>   5 files changed, 97 insertions(+), 20 deletions(-)
>>

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

[Wen Gu]

On 2024/8/6 18:49, Simon Horman wrote:
> 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.
> 

Yes.. I forgot to remove 'inline' when I moved these two helpers
from .h file to .c file. I will fix this in next version.

Thank you!

>> +{
>> +	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;
>> +	}
>> +}
>> +
> 
> ...
> 

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

[Wen Gu]

On 2024/8/6 18:49, Simon Horman wrote:
> 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).
> 

It makes sense. I will use a macro-local variable of smc_stats. Thank you!

>> +	} 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
>>
>>

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

[Simon Horman]

On Tue, Aug 06, 2024 at 09:07:40PM +0800, Wen Gu wrote:
> 
> 
> On 2024/8/6 18:49, Simon Horman wrote:
> > 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).
> > 
> 
> It makes sense. I will use a macro-local variable of smc_stats. Thank you!

Great, thanks.