net/smc: Optimizing performance in

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 accpet() 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.

Signed-off-by D. Wythe <[email protected]>

---
 net/smc/af_smc.c | 13 +++++++++++--
 net/smc/smc.h    |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

net/smc/af_smc.c +11 -2

index d5ea62b..1b40304 100644
@@ -59,6 +59,7 @@
 						 * creation on client
 						 */
 
+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 */
 

@@ -2124,7 +2125,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);
 	}
 }

@@ -2919,9 +2920,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)

@@ -2992,6 +2998,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:

@@ -3010,6 +3018,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);

net/smc/smc.h +0 -0

index 3d0b8e3..bd2f3dc 100644

net/smc/smc.h +0 -0

 extern struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
 extern struct workqueue_struct	*smc_close_wq;	/* wq for close work */
 

Quoted 2 lines

+extern struct workqueue_struct *smc_tcp_ls_wq; /* wq for tcp listen work */

I don't think this extern is needed, the work queue is only used within af_smc.c, right? Even the smc_hs_wq would not need to be extern, but this would be a future cleanup.

Quoted 3 lines

extern struct workqueue_struct *smc_hs_wq; /* wq for handshake work */
extern struct workqueue_struct *smc_close_wq; /* wq for close work */

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.

Signed-off-by D. Wythe <[email protected]>

---
changelog:
v2: fix compile warning

---
 net/smc/af_smc.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 net/smc/smc.h    |  4 ++++
 2 files changed, 47 insertions(+)

net/smc/af_smc.c +11 -0

index 1b40304..66a0e64 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) >

net/smc/af_smc.c +13 -0

+
+drop:
+	dst_release(dst);
+	tcp_listendrop(sk);
+	return NULL;
+}
+
 static struct smc_hashinfo smc_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
 };

@@ -1491,6 +1519,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);

@@ -2096,6 +2127,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;

@@ -2163,6 +2197,15 @@ static int smc_listen(struct socket *sock, int backlog)
 	smc->clcsock->sk->sk_data_ready = smc_clcsock_data_ready;

I am wondering if there would introduce more overhead, compared with original implement?

Quoted 34 lines

net/smc/af_smc.c +13 -0

+
+drop:
+	dst_release(dst);
+	tcp_listendrop(sk);
+	return NULL;
+}
+
  static struct smc_hashinfo smc_v4_hashinfo = {
  	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
  };

@@ -1491,6 +1519,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);

@@ -2096,6 +2127,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;

@@ -2163,6 +2197,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);

net/smc/af_smc.c static int smc_listen(struct socket *sock, int backlog) · line 2207

Quoted 8 lines

net/smc/af_smc.c +8 -0

+
+	/* 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;

Consider to save syn_recv_sock this field only? There seems no need to save this ops all.

Thank you,
Tony Lu

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

(quoted diff) +0 -0

@@ -19,3 +19,15 @@ config SMC_DIAG

Using a netlink knob to control behavior with static key should be more flexible. As I appended in the previous version of this patch.

Thank you,
Tony Lu

Quoted 2 lines · D. Wythe

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

(quoted diff) +0 -0

@@ -19,3 +19,15 @@ config SMC_DIAG

...

net/smc/Kconfig config SMC_DIAG · line 33

Quoted 11 lines

net/smc/Kconfig +11 -0

+if SMC
+
+config SMC_AUTO_FALLBACK
+	bool "SMC: automatic fallback to TCP"
+	default y
+	help
+	  Allow automatic fallback to TCP accroding to the pressure of SMC-R
+	  handshake process.
+
+	  If that's not what you except or unsure, say N.
+endif

Using a netlink knob to control behavior with static key should be more flexible. As I appended in the previous version of this patch.

Thank you,
Tony Lu

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?

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

Quoted 2 lines · D. Wythe

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

Cover letter subject: "Optimizing performance in" ??