[v2,net-next,1/3] net/smc: Make smc_tcp_listen_work() independent

Commit Message

D. Wythe Jan. 28, 2022, 2:44 p.m. UTC

From: "D. Wythe" <[e-mail]>

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 <[e-mail]>
---
 net/smc/af_smc.c | 13 +++++++++++--
 net/smc/smc.h    |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Karsten Graul Jan. 31, 2022, 12:45 p.m. UTC | #1

On 28/01/2022 15:44, D. Wythe wrote:
> From: "D. Wythe" <[e-mail]>
> 
> 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
                                                      ^^^
                                                      accept()
> 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 <[e-mail]>
> ---
>  net/smc/af_smc.c | 13 +++++++++++--
>  net/smc/smc.h    |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index d5ea62b..1b40304 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -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);
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 3d0b8e3..bd2f3dc 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -264,6 +264,7 @@ static inline struct smc_sock *smc_sk(const struct sock *sk)
>  	return (struct smc_sock *)sk;
>  }
>  
> +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.

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

D. Wythe Feb. 2, 2022, 12:53 p.m. UTC | #2

That's right, 'extern' is unnecessary, I'll remove it soon.

Looking forward for more advise.

Thanks.


>>   
>> +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.
> 
>>   extern struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
>>   extern struct workqueue_struct	*smc_close_wq;	/* wq for close work */
>>

Patch

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index d5ea62b..1b40304 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -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);
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 3d0b8e3..bd2f3dc 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -264,6 +264,7 @@  static inline struct smc_sock *smc_sk(const struct sock *sk)
 	return (struct smc_sock *)sk;
 }
 
+extern struct workqueue_struct	*smc_tcp_ls_wq;	/* wq for tcp listen work */
 extern struct workqueue_struct	*smc_hs_wq;	/* wq for handshake work */
 extern struct workqueue_struct	*smc_close_wq;	/* wq for close work */