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

[Karsten Graul <kgraul@linux.ibm.com>]

On 28/01/2022 15:44, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> 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 <alibuda@linux.alibaba.com>
> ---
>  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 */
>