Skip to contentSkip to conversation

net/smc: Optimizing performance in short-lived scenarios

PATCH Superseded D. Wythe 3 participants 23 messages Feb 8, 2022 – Feb 9, 2022

Subsystems (7)

v5 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
author maintainer
#14 lore raw

PATCH v5 5/5 net/smc: Add global configure for auto fallback by netlink

In reply to D. Wythe

From: "D. Wythe" <[email protected]>

Although we can control SMC auto fallback through socket options, which means that applications who need it must modify their code. It's quite troublesome for many existing applications. This patch modifies the global default value of auto fallback through netlink, providing a way to auto fallback without modifying any code for applications.

---
 include/uapi/linux/smc.h |  3 +++
 net/smc/af_smc.c         | 17 +++++++++++++++++
 net/smc/smc.h            |  7 +++++++
 net/smc/smc_core.c       |  2 ++
 net/smc/smc_netlink.c    | 10 ++++++++++
 5 files changed, 39 insertions(+)
include/uapi/linux/smc.h +3 -0
index 9f2cbf8..33f7fb8 100644
@@ -59,6 +59,8 @@ enum {
 	SMC_NETLINK_DUMP_SEID,
 	SMC_NETLINK_ENABLE_SEID,
 	SMC_NETLINK_DISABLE_SEID,
+	SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+	SMC_NETLINK_DISABLE_AUTO_FALLBACK,
 };
 
 /* SMC_GENL_FAMILY top level attributes */
@@ -85,6 +87,7 @@ enum {
 	SMC_NLA_SYS_LOCAL_HOST,		/* string */
 	SMC_NLA_SYS_SEID,		/* string */
 	SMC_NLA_SYS_IS_SMCR_V2,		/* u8 */
+	SMC_NLA_SYS_AUTO_FALLBACK,	/* u8 */
 	__SMC_NLA_SYS_MAX,
 	SMC_NLA_SYS_MAX = __SMC_NLA_SYS_MAX - 1
 };
net/smc/af_smc.c +17 -0
index c313561..4a25ce7 100644
@@ -59,6 +59,8 @@
 						 * creation on client
 						 */
 
+bool smc_auto_fallback;	/* default behavior for auto fallback, disable by default */
+
 static struct workqueue_struct	*smc_tcp_ls_wq;	/* wq for tcp listen work */
 struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
 struct workqueue_struct	*smc_close_wq;	/* wq for close work */
@@ -66,6 +68,18 @@
 static void smc_tcp_listen_work(struct work_struct *);
 static void smc_connect_work(struct work_struct *);
 
+int smc_enable_auto_fallback(struct sk_buff *skb, struct genl_info *info)
+{
+	WRITE_ONCE(smc_auto_fallback, true);
+	return 0;
+}
+
+int smc_disable_auto_fallback(struct sk_buff *skb, struct genl_info *info)
+{
+	WRITE_ONCE(smc_auto_fallback, false);
+	return 0;
+}
+
 static void smc_set_keepalive(struct sock *sk, int val)
 {
 	struct smc_sock *smc = smc_sk(sk);
@@ -3006,6 +3020,9 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
 	smc->use_fallback = false; /* assume rdma capability first */
 	smc->fallback_rsn = 0;
 
+	/* default behavior from smc_auto_fallback */
+	smc->auto_fallback = READ_ONCE(smc_auto_fallback);
+
 	rc = 0;
 	if (!clcsock) {
 		rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
net/smc/smc.h +7 -0
index a0bdf75..ac75fe8 100644
@@ -14,6 +14,7 @@
 #include <linux/socket.h>
 #include <linux/types.h>
 #include <linux/compiler.h> /* __aligned */
+#include <net/genetlink.h>
 #include <net/sock.h>
 
 #include "smc_ib.h"
@@ -336,4 +337,10 @@ void smc_fill_gid_list(struct smc_link_group *lgr,
 		       struct smc_gidlist *gidlist,
 		       struct smc_ib_device *known_dev, u8 *known_gid);
 
+extern bool smc_auto_fallback; /* default behavior for auto fallback */
+
+/* smc_auto_fallback setter for netlink */
+int smc_enable_auto_fallback(struct sk_buff *skb, struct genl_info *info);
+int smc_disable_auto_fallback(struct sk_buff *skb, struct genl_info *info);
+
 #endif	/* __SMC_H */
net/smc/smc_core.c +2 -0
index 29525d0..cc9a398 100644
@@ -248,6 +248,8 @@ int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb)
 		goto errattr;
 	if (nla_put_u8(skb, SMC_NLA_SYS_IS_SMCR_V2, true))
 		goto errattr;
+	if (nla_put_u8(skb, SMC_NLA_SYS_AUTO_FALLBACK, smc_auto_fallback))
+		goto errattr;
 	smc_clc_get_hostname(&host);
 	if (host) {
 		memcpy(hostname, host, SMC_MAX_HOSTNAME_LEN);
net/smc/smc_netlink.c +10 -0
index f13ab06..a7de517 100644
@@ -111,6 +111,16 @@
 		.flags = GENL_ADMIN_PERM,
 		.doit = smc_nl_disable_seid,
 	},
+	{
+		.cmd = SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_enable_auto_fallback,
+	},
+	{
+		.cmd = SMC_NETLINK_DISABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_disable_auto_fallback,
+	},
 };
 
 static const struct nla_policy smc_gen_nl_policy[2] = {
Signature · 1 line
1.8.3.1

D. Wythe

author maintainer
#0 lore raw

v5 cover net/smc: Optimizing performance in short-lived scenarios

From: "D. Wythe" <[email protected]>

This patch set aims to optimizing performance of SMC in short-lived links scenarios, which is quite unsatisfactory right now.

In our benchmark, we test it with follow scripts:

./wrk -c 10000 -t 4 -H 'Connection: Close' -d 20 http://smc-server

Current performance figures like that:

Running 20s test @ http://11.213.45.6
4 threads and 10000 connections
4956 requests in 20.06s, 3.24MB read
Socket errors: connect 0, read 0, write 672, timeout 0

Requests/sec:    247.07
Transfer/sec:    165.28KB

There are many reasons for this phenomenon, this patch set doesn't solve it all though, but it can be well alleviated with it in.

Patch 1/5 (Make smc_tcp_listen_work() independent) :

Separate smc_tcp_listen_work() from smc_listen_work(), make them independent of each other, the busy SMC handshake can not affect new TCP connections visit any more. Avoid discarding a large number of TCP connections after being overstock, which is undoubtedly raise the connection establishment time.

Patch 2/5 (Limits SMC backlog connections):

Since patch 1 has separated smc_tcp_listen_work() from smc_listen_work(), an unrestricted TCP accept have come into being. This patch try to put a limit on SMC backlog connections refers to implementation of TCP.

Patch 3/5 (Fallback when SMC handshake workqueue congested):

Considering the complexity of SMC handshake right now, in short-lived links scenarios, this may not be the main scenario of SMC though, it's performance is still quite poor. This Patch try to provide auto fallback case when SMC handshake workqueue congested, which is the sign of SMC handshake stacking in our opinion.

Patch 4/5 (Dynamic control SMC auto fallback by socket options)

This patch allow applications dynamically control the ability of SMC auto fallback. Since SMC don't support set SMC socket option before, this patch also have to support SMC's owns socket options.

Patch 5/5 (Add global configure for auto fallback by netlink)

This patch provides a way to get benefit of auto fallback without modifying any code for applications, which is quite useful for most existing applications.

After this patch set, performance figures like that:

Running 20s test @ http://11.213.45.6
4 threads and 10000 connections
693253 requests in 20.10s, 452.88MB read

Requests/sec:  34488.13
Transfer/sec:     22.53MB

That's a quite well performance improvement, about to 6 to 7 times in my environment.
---
changelog:
v2 -> v1:
- fix compile warning
- fix invalid dependencies in kconfig
v3 -> v2:
- correct spelling mistakes
- fix useless variable declare
v4 -> v3
- make smc_tcp_ls_wq be static
v5 -> v4
- add dynamic control for SMC auto fallback by socket options
- add global configure for SMC auto fallback through netlink
---
D. Wythe (5):
net/smc: Make smc_tcp_listen_work() independent
net/smc: Limits backlog connections
net/smc: Fallback when handshake workqueue congested net/smc: Dynamic control auto fallback by socket options net/smc: Add global configure for auto fallback by netlink

 include/linux/socket.h   |   1 +
 include/linux/tcp.h      |   1 +
 include/uapi/linux/smc.h |   7 +++
 net/ipv4/tcp_input.c     |   3 +-
 net/smc/af_smc.c         | 158 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/smc/smc.h            |  12 ++++
 net/smc/smc_core.c       |   2 +
 net/smc/smc_netlink.c    |  10 +++
 8 files changed, 191 insertions(+), 3 deletions(-)
Signature · 1 line
1.8.3.1
author maintainer
#8 lore raw

PATCH v5 3/5 net/smc: Fallback when handshake workqueue congested

From: "D. Wythe" <[email protected]>

This patch intends to provide a mechanism to allow automatic fallback to TCP according to the pressure of SMC handshake process. At present, frequent visits will cause the incoming connections to be backlogged in SMC handshake queue, raise the connections established time. Which is quite unacceptable for those applications who base on short lived connections.

There are two ways to implement this mechanism:

1. Fallback when TCP established.
2. Fallback before TCP established.

In the first way, we need to wait and receive CLC messages that the client will potentially send, and then actively reply with a decline message, in a sense, which is also a sort of SMC handshake, affect the connections established time on its way.

In the second way, the only problem is that we need to inject SMC logic into TCP when it is about to reply the incoming SYN, since we already do that, it's seems not a problem anymore. And advantage is obvious, few additional processes are required to complete the fallback.

This patch use the second way.

---
 include/linux/tcp.h  |  1 +
 net/ipv4/tcp_input.c |  3 ++-
 net/smc/af_smc.c     | 18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)
include/linux/tcp.h +1 -0
index 78b91bb..1c4ae5d 100644
@@ -394,6 +394,7 @@ struct tcp_sock {
 	bool	is_mptcp;
 #endif
 #if IS_ENABLED(CONFIG_SMC)
+	bool	(*smc_in_limited)(const struct sock *sk);
 	bool	syn_smc;	/* SYN includes SMC */
 #endif
 
net/ipv4/tcp_input.c +2 -1
index af94a6d..e817ec6 100644
@@ -6703,7 +6703,8 @@ static void tcp_openreq_init(struct request_sock *req,
 	ireq->ir_num = ntohs(tcp_hdr(skb)->dest);
 	ireq->ir_mark = inet_request_mark(sk, skb);
 #if IS_ENABLED(CONFIG_SMC)
-	ireq->smc_ok = rx_opt->smc_ok;
+	ireq->smc_ok = rx_opt->smc_ok && !(tcp_sk(sk)->smc_in_limited &&
+			tcp_sk(sk)->smc_in_limited(sk));
 #endif
 }
 
net/smc/af_smc.c +18 -0
index ebfce3d..8175f60 100644
@@ -101,6 +101,22 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff
 	return NULL;
 }
 
+static bool smc_is_in_limited(const struct sock *sk)
+{
+	const struct smc_sock *smc;
+
+	smc = (const struct smc_sock *)
+		((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+
+	if (!smc)
+		return true;
+
+	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
+		return true;
+
+	return false;
+}
+
 static struct smc_hashinfo smc_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
 };
@@ -2309,6 +2325,8 @@ static int smc_listen(struct socket *sock, int backlog)
 
 	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
 
+	tcp_sk(smc->clcsock->sk)->smc_in_limited = smc_is_in_limited;
+
 	rc = kernel_listen(smc->clcsock, backlog);
 	if (rc) {
 		smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;
Signature · 1 line
1.8.3.1
author maintainer
#4 lore raw

PATCH v5 2/5 net/smc: Limit backlog connections

In reply to D. Wythe

From: "D. Wythe" <[email protected]>

Current implementation does not handling backlog semantics, one potential risk is that server will be flooded by infinite amount connections, even if client was SMC-incapable.

This patch works to put a limit on backlog connections, referring to the TCP implementation, we divides SMC connections into two categories:

1. Half SMC connection, which includes all TCP established while SMC not connections.

2. Full SMC connection, which includes all SMC established connections.

For half SMC connection, since all half SMC connections starts with TCP established, we can achieve our goal by put a limit before TCP established. Refer to the implementation of TCP, this limits will based on not only the half SMC connections but also the full connections, which is also a constraint on full SMC connections.

For full SMC connections, although we know exactly where it starts, it's quite hard to put a limit before it. The easiest way is to block wait before receive SMC confirm CLC message, while it's under protection by smc_server_lgr_pending, a global lock, which leads this limit to the entire host instead of a single listen socket. Another way is to drop the full connections, but considering the cast of SMC connections, we prefer to keep full SMC connections.

Even so, the limits of full SMC connections still exists, see commits about half SMC connection below.

After this patch, the limits of backend connection shows like:

For SMC:

1. Client with SMC-capability can makes 2 * backlog full SMC connections or 1 * backlog half SMC connections and 1 * backlog full SMC connections at most.

2. Client without SMC-capability can only makes 1 * backlog half TCP connections and 1 * backlog full TCP connections.

---
 net/smc/af_smc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 net/smc/smc.h    |  4 ++++
 2 files changed, 47 insertions(+)
net/smc/af_smc.c +43 -0
index 4969ac8..ebfce3d 100644
@@ -73,6 +73,34 @@ static void smc_set_keepalive(struct sock *sk, int val)
 	smc->clcsock->sk->sk_prot->keepalive(smc->clcsock->sk, val);
 }
 
+static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
+					  struct request_sock *req,
+					  struct dst_entry *dst,
+					  struct request_sock *req_unhash,
+					  bool *own_req)
+{
+	struct smc_sock *smc;
+
+	smc = (struct smc_sock *)((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+
+	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->smc_pendings) >
+				sk->sk_max_ack_backlog)
+		goto drop;
+
+	if (sk_acceptq_is_full(&smc->sk)) {
+		NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
+		goto drop;
+	}
+
+	/* passthrough to origin syn recv sock fct */
+	return smc->ori_af_ops->syn_recv_sock(sk, skb, req, dst, req_unhash, own_req);
+
+drop:
+	dst_release(dst);
+	tcp_listendrop(sk);
+	return NULL;
+}
+
 static struct smc_hashinfo smc_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
 };
@@ -1595,6 +1623,9 @@ static void smc_listen_out(struct smc_sock *new_smc)
 	struct smc_sock *lsmc = new_smc->listen_smc;
 	struct sock *newsmcsk = &new_smc->sk;
 
+	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
+		atomic_dec(&lsmc->smc_pendings);
+
 	if (lsmc->sk.sk_state == SMC_LISTEN) {
 		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
 		smc_accept_enqueue(&lsmc->sk, newsmcsk);
@@ -2200,6 +2231,9 @@ static void smc_tcp_listen_work(struct work_struct *work)
 		if (!new_smc)
 			continue;
 
+		if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
+			atomic_inc(&lsmc->smc_pendings);
+
 		new_smc->listen_smc = lsmc;
 		new_smc->use_fallback = lsmc->use_fallback;
 		new_smc->fallback_rsn = lsmc->fallback_rsn;
@@ -2266,6 +2300,15 @@ static int smc_listen(struct socket *sock, int backlog)
 	smc->clcsock->sk->sk_data_ready = smc_clcsock_data_ready;
 	smc->clcsock->sk->sk_user_data =
 		(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
+
+	/* save origin ops */
+	smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;
+
+	smc->af_ops = *smc->ori_af_ops;
+	smc->af_ops.syn_recv_sock = smc_tcp_syn_recv_sock;
+
+	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
+
 	rc = kernel_listen(smc->clcsock, backlog);
 	if (rc) {
 		smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;
net/smc/smc.h +4 -0
index 37b2001..5e5e38d 100644
@@ -252,6 +252,10 @@ struct smc_sock {				/* smc sock container */
 	bool			use_fallback;	/* fallback to tcp */
 	int			fallback_rsn;	/* reason for fallback */
 	u32			peer_diagnosis; /* decline reason from peer */
+	atomic_t                smc_pendings;   /* pending smc connections */
+	struct inet_connection_sock_af_ops		af_ops;
+	const struct inet_connection_sock_af_ops	*ori_af_ops;
+						/* origin af ops */
 	int			sockopt_defer_accept;
 						/* sockopt TCP_DEFER_ACCEPT
 						 * value
Signature · 1 line
1.8.3.1
author maintainer
#1 lore raw

PATCH v5 1/5 net/smc: Make smc_tcp_listen_work() independent

In reply to D. Wythe

From: "D. Wythe" <[email protected]>

In multithread and 10K connections benchmark, the backend TCP connection established very slowly, and lots of TCP connections stay in SYN_SENT state.

Client: smc_run wrk -c 10000 -t 4 http://server

the netstate of server host shows like:
145042 times the listen queue of a socket overflowed 145042 SYNs to LISTEN sockets dropped

One reason of this issue is that, since the smc_tcp_listen_work() shared the same workqueue (smc_hs_wq) with smc_listen_work(), while the smc_listen_work() do blocking wait for smc connection established. Once the workqueue became congested, it's will block the accept() from TCP listen.

This patch creates a independent workqueue(smc_tcp_ls_wq) for smc_tcp_listen_work(), separate it from smc_listen_work(), which is quite acceptable considering that smc_tcp_listen_work() runs very fast.

---
 net/smc/af_smc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
net/smc/af_smc.c +11 -2
index 00b2e9d..4969ac8 100644
@@ -59,6 +59,7 @@
 						 * creation on client
 						 */
 
+static struct workqueue_struct	*smc_tcp_ls_wq;	/* wq for tcp listen work */
 struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
 struct workqueue_struct	*smc_close_wq;	/* wq for close work */
 
@@ -2227,7 +2228,7 @@ static void smc_clcsock_data_ready(struct sock *listen_clcsock)
 	lsmc->clcsk_data_ready(listen_clcsock);
 	if (lsmc->sk.sk_state == SMC_LISTEN) {
 		sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
-		if (!queue_work(smc_hs_wq, &lsmc->tcp_listen_work))
+		if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work))
 			sock_put(&lsmc->sk);
 	}
 }
@@ -3024,9 +3025,14 @@ static int __init smc_init(void)
 		goto out_nl;
 
 	rc = -ENOMEM;
+
+	smc_tcp_ls_wq = alloc_workqueue("smc_tcp_ls_wq", 0, 0);
+	if (!smc_tcp_ls_wq)
+		goto out_pnet;
+
 	smc_hs_wq = alloc_workqueue("smc_hs_wq", 0, 0);
 	if (!smc_hs_wq)
-		goto out_pnet;
+		goto out_alloc_tcp_ls_wq;
 
 	smc_close_wq = alloc_workqueue("smc_close_wq", 0, 0);
 	if (!smc_close_wq)
@@ -3097,6 +3103,8 @@ static int __init smc_init(void)
 	destroy_workqueue(smc_close_wq);
 out_alloc_hs_wq:
 	destroy_workqueue(smc_hs_wq);
+out_alloc_tcp_ls_wq:
+	destroy_workqueue(smc_tcp_ls_wq);
 out_pnet:
 	smc_pnet_exit();
 out_nl:
@@ -3115,6 +3123,7 @@ static void __exit smc_exit(void)
 	smc_core_exit();
 	smc_ib_unregister_client();
 	destroy_workqueue(smc_close_wq);
+	destroy_workqueue(smc_tcp_ls_wq);
 	destroy_workqueue(smc_hs_wq);
 	proto_unregister(&smc_proto6);
 	proto_unregister(&smc_proto);
Signature · 1 line
1.8.3.1
author maintainer
#9 lore raw

PATCH v5 4/5 net/smc: Dynamic control auto fallback by socket options

In reply to D. Wythe

From: "D. Wythe" <[email protected]>

This patch aims to add dynamic control for SMC auto fallback, since we don't have socket option level for SMC yet, which requires we need to implement it at the same time.

This patch does the following:

- add new socket option level: SOL_SMC.
- add new SMC socket option: SMC_AUTO_FALLBACK.
- provide getter/setter for SMC socket options.

---
 include/linux/socket.h   |  1 +
 include/uapi/linux/smc.h |  4 +++
 net/smc/af_smc.c         | 69 +++++++++++++++++++++++++++++++++++++++++++++++-
 net/smc/smc.h            |  1 +
 4 files changed, 74 insertions(+), 1 deletion(-)
include/linux/socket.h +1 -0
index 8ef26d8..6f85f5d 100644
@@ -366,6 +366,7 @@ struct ucred {
 #define SOL_XDP		283
 #define SOL_MPTCP	284
 #define SOL_MCTP	285
+#define SOL_SMC		286
 
 /* IPX options */
 #define IPX_TYPE	1
include/uapi/linux/smc.h +4 -0
index 6c2874f..9f2cbf8 100644
@@ -284,4 +284,8 @@ enum {
 	__SMC_NLA_SEID_TABLE_MAX,
 	SMC_NLA_SEID_TABLE_MAX = __SMC_NLA_SEID_TABLE_MAX - 1
 };
+
+/* SMC socket options */
+#define SMC_AUTO_FALLBACK 1	/* allow auto fallback to TCP */
+
 #endif /* _UAPI_LINUX_SMC_H */
net/smc/af_smc.c +68 -1

93 lines not on this page · raw message

net/smc/smc.h +1 -0
index 5e5e38d..a0bdf75 100644
@@ -249,6 +249,7 @@ struct smc_sock {				/* smc sock container */
 	struct work_struct	smc_listen_work;/* prepare new accept socket */
 	struct list_head	accept_q;	/* sockets to be accepted */
 	spinlock_t		accept_q_lock;	/* protects accept_q */
+	bool			auto_fallback;	/* auto fallabck to tcp */
 	bool			use_fallback;	/* fallback to tcp */
 	int			fallback_rsn;	/* reason for fallback */
 	u32			peer_diagnosis; /* decline reason from peer */
Signature · 1 line
1.8.3.1
#22 lore raw
In reply to D. Wythe
Quoted 4 lines · D. Wythe

From: "D. Wythe" <[email protected]>

This patch set aims to optimizing performance of SMC in short-lived links scenarios, which is quite unsatisfactory right now.

Thanks for the submission, we are discussing a few changes but overall it looks good to us! I will send comments now but some more tomorrow after we discussed remaining details.

Thank you!

#2 lore raw

Re: [PATCH net-next v5 1/5] net/smc: Make smc_tcp_listen_work() independent

In reply to D. Wythe

net/smc/af_smc.c static void smc_clcsock_data_ready(struct sock *listen_clcsock) · line 2232

Quoted 11 lines · D. Wythe
net/smc/af_smc.c +2 -1
+static struct workqueue_struct	*smc_tcp_ls_wq;	/* wq for tcp listen work */
 struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
 struct workqueue_struct	*smc_close_wq;	/* wq for close work */
@@ -2227,7 +2228,7 @@ static void smc_clcsock_data_ready(struct sock *listen_clcsock)
 	lsmc->clcsk_data_ready(listen_clcsock);
 	if (lsmc->sk.sk_state == SMC_LISTEN) {
 		sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
-		if (!queue_work(smc_hs_wq, &lsmc->tcp_listen_work))
+		if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work))
 			sock_put(&lsmc->sk);

It works well this way, but given the fact that there is one tcp_listen worker per listen socket and these workers finish relatively quickly, wouldn't it be okay to use the system_wq instead of using an own queue? But I have no strong opinion about that...

#10 lore raw

Re: [PATCH net-next v5 4/5] net/smc: Dynamic control auto fallback by socket options

In reply to D. Wythe
Quoted 5 lines · D. Wythe

From: "D. Wythe" <[email protected]>

This patch aims to add dynamic control for SMC auto fallback, since we don't have socket option level for SMC yet, which requires we need to implement it at the same time.

Quoted 3 lines · In your response to the v2 version of this series you wrote:

After some trial and thought, I found that the scope of netlink control is too large, we should limit the scope to socket. Adding a socket option may be a better choice, what do you think?

I want to understand why this socket option is required, who needs it and why. What were your trials and thoughts, did you see any problems with the global switch via the netlink interface?

#5 lore raw

Re: [PATCH net-next v5 2/5] net/smc: Limit backlog connections

In reply to D. Wythe
Quoted 5 lines · D. Wythe

From: "D. Wythe" <[email protected]>

Current implementation does not handling backlog semantics, one potential risk is that server will be flooded by infinite amount connections, even if client was SMC-incapable.

In this patch you count the number of inflight SMC handshakes as pending and check them against the defined max_backlog. I really like this improvement.

There is another queue in af_smc.c, the smc accept queue and any new client socket that completed the handshake process is enqueued there (in smc_accept_enqueue() ) and is waiting to get accepted by the user space application. To apply the correct semantics here, I think the number of sockets waiting in the smc accept queue should also be counted as backlog connections, right? I see no limit for this queue now. What do you think?

author maintainer
#3 lore raw

Re: [PATCH net-next v5 1/5] net/smc: Make smc_tcp_listen_work() independent

In reply to Karsten Graul

It is indeed okay to use system_wq at present. Dues to the load balancing issues we found, queue_work() always submits tasks to the worker on the current CPU. tcp_listen_work() execution once may submit a large number of tasks to the worker of the current CPU, causing unnecessary pending, even though worker on other CPU are totaly free. I was plan to make tcp_listen_work() blocked wait on worker of every CPU, so I create a new workqueue, and that's the only reason for it. But this problem is not very urgent, and I don't have strong opinion too...

Quoted 16 lines · Karsten Graul

net/smc/af_smc.c static void smc_clcsock_data_ready(struct sock *listen_clcsock) · line 2232

Quoted 11 lines · D. Wythe
net/smc/af_smc.c +2 -1
+static struct workqueue_struct	*smc_tcp_ls_wq;	/* wq for tcp listen work */
  struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
  struct workqueue_struct	*smc_close_wq;	/* wq for close work */
@@ -2227,7 +2228,7 @@ static void smc_clcsock_data_ready(struct sock *listen_clcsock)
  	lsmc->clcsk_data_ready(listen_clcsock);
  	if (lsmc->sk.sk_state == SMC_LISTEN) {
  		sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
-		if (!queue_work(smc_hs_wq, &lsmc->tcp_listen_work))
+		if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work))
  			sock_put(&lsmc->sk);

It works well this way, but given the fact that there is one tcp_listen worker per listen socket and these workers finish relatively quickly, wouldn't it be okay to use the system_wq instead of using an own queue? But I have no strong opinion about that...

author maintainer
#11 lore raw

Re: [PATCH net-next v5 4/5] net/smc: Dynamic control auto fallback by socket options

In reply to Karsten Graul

Some of our servers have different service types on different ports.
A global switch cannot control different service ports individually in this case。In fact, it has nothing to do with using netlink or not. Socket options is the first solution comes to my mind in that case,I don't know if there is any other better way。

Looks for you suggestions.
Thanks.

Quoted 16 lines · Karsten Graul
Quoted 5 lines · D. Wythe

From: "D. Wythe" <[email protected]>

This patch aims to add dynamic control for SMC auto fallback, since we don't have socket option level for SMC yet, which requires we need to implement it at the same time.

Quoted 3 lines · In your response to the v2 version of this series you wrote:

After some trial and thought, I found that the scope of netlink control is too large, we should limit the scope to socket. Adding a socket option may be a better choice, what do you think?

I want to understand why this socket option is required, who needs it and why. What were your trials and thoughts, did you see any problems with the global switch via the netlink interface?

author maintainer
#6 lore raw

Re: [PATCH net-next v5 2/5] net/smc: Limit backlog connections

In reply to Karsten Graul

There are indirectly limits on smc accept queue with following code.

+ if (sk_acceptq_is_full(&smc->sk)) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
+ goto drop;
+ }

In fact, we treat the connections in smc accept queue as Full establisted connection. As I wrote in patch commits, there are trade-offs to this implemets.

Thanks.

Quoted 16 lines · Karsten Graul
Quoted 5 lines · D. Wythe

From: "D. Wythe" <[email protected]>

Current implementation does not handling backlog semantics, one potential risk is that server will be flooded by infinite amount connections, even if client was SMC-incapable.

In this patch you count the number of inflight SMC handshakes as pending and check them against the defined max_backlog. I really like this improvement.

There is another queue in af_smc.c, the smc accept queue and any new client socket that completed the handshake process is enqueued there (in smc_accept_enqueue() ) and is waiting to get accepted by the user space application. To apply the correct semantics here, I think the number of sockets waiting in the smc accept queue should also be counted as backlog connections, right? I see no limit for this queue now. What do you think?

#7 lore raw

Re: [PATCH net-next v5 2/5] net/smc: Limit backlog connections

Quoted 11 lines · D. Wythe

There are indirectly limits on smc accept queue with following code.

+    if (sk_acceptq_is_full(&smc->sk)) {
+        NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS); +        goto drop;
+    }

In fact, we treat the connections in smc accept queue as Full establisted connection. As I wrote in patch commits, there are trade-offs to this implemets.

Thanks for the clarification, I got your point. You refer to the call to sk_acceptq_added() in smc_accept_enqueue().

#12 lore raw

Re: [PATCH net-next v5 4/5] net/smc: Dynamic control auto fallback by socket options

In reply to D. Wythe
Quoted 4 lines · D. Wythe

Some of our servers have different service types on different ports.
A global switch cannot control different service ports individually in this case。In fact, it has nothing to do with using netlink or not. Socket options is the first solution comes to my mind in that case,I don't know if there is any other better way。

I try to understand why you think it is needed to handle different service types differently. As you wrote

Quoted 1 line

After some trial and thought, I found that the scope of netlink control is too large

please explain what you found out. I don't doubt about netlink or socket option here, its all about why a global switch for this behavior isn't good enough.

reviewer
#15 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to D. Wythe

net/smc/smc_core.c int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb) · line 251

Quoted 7 lines · D. Wythe

From: "D. Wythe" <[email protected]>

net/smc/smc_core.c +1 -0
@@ -248,6 +248,8 @@ int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb)
 		goto errattr;
 	if (nla_put_u8(skb, SMC_NLA_SYS_IS_SMCR_V2, true))
 		goto errattr;
+	if (nla_put_u8(skb, SMC_NLA_SYS_AUTO_FALLBACK, smc_auto_fallback))

READ_ONCE(smc_auto_fallback) ?

Quoted 13 lines
net/smc/smc_core.c +1 -0
+		goto errattr;
 	smc_clc_get_hostname(&host);
 	if (host) {
 		memcpy(hostname, host, SMC_MAX_HOSTNAME_LEN);
net/smc/smc_netlink.c +1 -0
index f13ab06..a7de517 100644
@@ -111,6 +111,16 @@
 		.flags = GENL_ADMIN_PERM,
 		.doit = smc_nl_disable_seid,
 	},
+	{

net/smc/smc_netlink.c line 124

Quoted 10 lines
net/smc/smc_netlink.c +9 -0
+		.cmd = SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_enable_auto_fallback,
+	},
+	{
+		.cmd = SMC_NETLINK_DISABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_disable_auto_fallback,
+	},
 };

Consider adding the separated cmd to query the status of this config, just as SEID does?

It is common to check this value after user-space setted. Combined with sys_info maybe a little heavy in this scene.

Thanks,
Tony Lu

author maintainer
#13 lore raw

Re: [PATCH net-next v5 4/5] net/smc: Dynamic control auto fallback by socket options

In reply to Karsten Graul

When a large number of connections are influx, the long-connection service has a much higher tolerance for smc queuing time than the short-link service. For the long-connection service, more SMC connections are more important than faster connection establishment, the auto fallback is quite meaningless and unexpected to them, while the short-link connection service is in the opposite. When a host has both types of services below, a global switch cannot works in that case. what do you think?

Hope for you reply.

Thanks.

Quoted 13 lines · Karsten Graul
Quoted 4 lines · D. Wythe

Some of our servers have different service types on different ports.
A global switch cannot control different service ports individually in this case。In fact, it has nothing to do with using netlink or not. Socket options is the first solution comes to my mind in that case,I don't know if there is any other better way。

I try to understand why you think it is needed to handle different service types differently. As you wrote

Quoted 1 line

After some trial and thought, I found that the scope of netlink control is too large

please explain what you found out. I don't doubt about netlink or socket option here, its all about why a global switch for this behavior isn't good enough.

reviewer
#18 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to D. Wythe
Quoted 44 lines · D. Wythe

From: "D. Wythe" <[email protected]>

Although we can control SMC auto fallback through socket options, which means that applications who need it must modify their code. It's quite troublesome for many existing applications. This patch modifies the global default value of auto fallback through netlink, providing a way to auto fallback without modifying any code for applications.

---
 include/uapi/linux/smc.h |  3 +++
 net/smc/af_smc.c         | 17 +++++++++++++++++
 net/smc/smc.h            |  7 +++++++
 net/smc/smc_core.c       |  2 ++
 net/smc/smc_netlink.c    | 10 ++++++++++
 5 files changed, 39 insertions(+)
include/uapi/linux/smc.h +3 -0
index 9f2cbf8..33f7fb8 100644
@@ -59,6 +59,8 @@ enum {
 	SMC_NETLINK_DUMP_SEID,
 	SMC_NETLINK_ENABLE_SEID,
 	SMC_NETLINK_DISABLE_SEID,
+	SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+	SMC_NETLINK_DISABLE_AUTO_FALLBACK,
 };
 
 /* SMC_GENL_FAMILY top level attributes */
@@ -85,6 +87,7 @@ enum {
 	SMC_NLA_SYS_LOCAL_HOST,		/* string */
 	SMC_NLA_SYS_SEID,		/* string */
 	SMC_NLA_SYS_IS_SMCR_V2,		/* u8 */
+	SMC_NLA_SYS_AUTO_FALLBACK,	/* u8 */
 	__SMC_NLA_SYS_MAX,
 	SMC_NLA_SYS_MAX = __SMC_NLA_SYS_MAX - 1
 };
net/smc/af_smc.c +0 -0
index c313561..4a25ce7 100644

net/smc/af_smc.c line 62

Quoted 4 lines
net/smc/af_smc.c +1 -0
@@ -59,6 +59,8 @@
 						 * creation on client
 						 */
 
+bool smc_auto_fallback;	/* default behavior for auto fallback, disable by default */

SMC supports net namespace, it would be better to provide a per net-namespace switch. Generally, one container has one application, runs different workload that is different from others. So the behavior could be different, such as high-performance (don't fallback for limit) or TCP transparent replacement (limit if needed).

Thank you,
Tony Lu

author maintainer
#16 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to Tony Lu
Quoted 10 lines · Tony Lu

net/smc/smc_core.c int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb) · line 251

Quoted 7 lines · D. Wythe

From: "D. Wythe" <[email protected]>

net/smc/smc_core.c +1 -0
@@ -248,6 +248,8 @@ int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb)
  		goto errattr;
  	if (nla_put_u8(skb, SMC_NLA_SYS_IS_SMCR_V2, true))
  		goto errattr;
+	if (nla_put_u8(skb, SMC_NLA_SYS_AUTO_FALLBACK, smc_auto_fallback))

READ_ONCE(smc_auto_fallback) ?

No READ_ONCE() will cause ?

Quoted 27 lines
Quoted 13 lines
net/smc/smc_core.c +1 -0
+		goto errattr;
  	smc_clc_get_hostname(&host);
  	if (host) {
  		memcpy(hostname, host, SMC_MAX_HOSTNAME_LEN);
net/smc/smc_netlink.c +1 -0
index f13ab06..a7de517 100644
@@ -111,6 +111,16 @@
  		.flags = GENL_ADMIN_PERM,
  		.doit = smc_nl_disable_seid,
  	},
+	{

net/smc/smc_netlink.c line 124

Quoted 10 lines
net/smc/smc_netlink.c +9 -0
+		.cmd = SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_enable_auto_fallback,
+	},
+	{
+		.cmd = SMC_NETLINK_DISABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_disable_auto_fallback,
+	},
  };

Consider adding the separated cmd to query the status of this config, just as SEID does?

Quoted 2 lines

It is common to check this value after user-space setted. Combined with sys_info maybe a little heavy in this scene.

Add a independent dumpit is quite okay, but is there have really scenarios that access this value frequently? With more and more such switches in the future, is is necessary for us to repeat on each switch ? I do have a plan to put them unified within a NLA attributes, but I don't have much time yet.

reviewer
#20 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to D. Wythe
Quoted 3 lines · D. Wythe

I don't think this is necessary, since we already have socket options. Is there any scenario that the socket options and global switch can not cover?

When transparently replacing the whole container's TCP connections, we cannot touch the user's application, and have to replace their connections to SMC. It is common for container environment, different containers will run different applications.

Most of TCP knob is per net-namespace, it could be better for us to do it from the beginning.

Thanks,
Tony Lu

author maintainer
#19 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to Tony Lu

I don't think this is necessary, since we already have socket options. Is there any scenario that the socket options and global switch can not cover?

Thanks.

Quoted 5 lines · Tony Lu

SMC supports net namespace, it would be better to provide a per net-namespace switch. Generally, one container has one application, runs different workload that is different from others. So the behavior could be different, such as high-performance (don't fallback for limit) or TCP transparent replacement (limit if needed).

author maintainer
#21 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to Tony Lu

Copy that, there are indeed some problems for the container environment. I'll try work on it.

Thanks.

Quoted 14 lines · Tony Lu
Quoted 3 lines · D. Wythe

I don't think this is necessary, since we already have socket options. Is there any scenario that the socket options and global switch can not cover?

When transparently replacing the whole container's TCP connections, we cannot touch the user's application, and have to replace their connections to SMC. It is common for container environment, different containers will run different applications.

Most of TCP knob is per net-namespace, it could be better for us to do it from the beginning.

Thanks,
Tony Lu

reviewer
#17 lore raw

Re: [PATCH net-next v5 5/5] net/smc: Add global configure for auto fallback by netlink

In reply to D. Wythe
Quoted 15 lines · D. Wythe
Quoted 10 lines · Tony Lu

net/smc/smc_core.c int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb) · line 251

Quoted 7 lines · D. Wythe

From: "D. Wythe" <[email protected]>

net/smc/smc_core.c +1 -0
@@ -248,6 +248,8 @@ int smc_nl_get_sys_info(struct sk_buff *skb, struct netlink_callback *cb)
  		goto errattr;
  	if (nla_put_u8(skb, SMC_NLA_SYS_IS_SMCR_V2, true))
  		goto errattr;
+	if (nla_put_u8(skb, SMC_NLA_SYS_AUTO_FALLBACK, smc_auto_fallback))

READ_ONCE(smc_auto_fallback) ?

Quoted 1 line

No READ_ONCE() will cause ?

Make sure that we read the current value.

Quoted 28 lines
Quoted 28 lines
Quoted 14 lines
net/smc/smc_core.c +1 -0
+		goto errattr;
  	smc_clc_get_hostname(&host);
  	if (host) {
  		memcpy(hostname, host, SMC_MAX_HOSTNAME_LEN);
net/smc/smc_netlink.c +1 -0
index f13ab06..a7de517 100644
@@ -111,6 +111,16 @@
  		.flags = GENL_ADMIN_PERM,
  		.doit = smc_nl_disable_seid,
  	},
+	{

net/smc/smc_netlink.c line 124

Quoted 10 lines
net/smc/smc_netlink.c +9 -0
+		.cmd = SMC_NETLINK_ENABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_enable_auto_fallback,
+	},
+	{
+		.cmd = SMC_NETLINK_DISABLE_AUTO_FALLBACK,
+		.flags = GENL_ADMIN_PERM,
+		.doit = smc_disable_auto_fallback,
+	},
  };

Consider adding the separated cmd to query the status of this config, just as SEID does?

Quoted 8 lines
Quoted 2 lines

It is common to check this value after user-space setted. Combined with sys_info maybe a little heavy in this scene.

Add a independent dumpit is quite okay, but is there have really scenarios that access this value frequently? With more and more such switches in the future, is is necessary for us to repeat on each switch ? I do have a plan to put them unified within a NLA attributes, but I don't have much time yet.

Yes, I think spreading them make code clean, and we can keep ABI compatibility if we have more than one interface. If we want to change one knob, we can change itself functions and data structures. Also, it makes userspace tools easy to maintainer. TCP's procfs, like /proc/net/netstat, is a summary knob, but not easy to parse and extend. Given that we choose modern netlink, we can avoid it from the beginning.

Thanks,
Tony Lu