Re: [PATCH net-next 1/3] net/smc: Send directly when TCP_CORK is cleared

[Stefan Raspl <raspl@linux.ibm.com>]

On 1/30/22 19:02, Tony Lu wrote:
> According to the man page of TCP_CORK [1], if set, don't send out
> partial frames. All queued partial frames are sent when option is
> cleared again.
> 
> When applications call setsockopt to disable TCP_CORK, this call is
> protected by lock_sock(), and tries to mod_delayed_work() to 0, in order
> to send pending data right now. However, the delayed work smc_tx_work is
> also protected by lock_sock(). There introduces lock contention for
> sending data.
> 
> To fix it, send pending data directly which acts like TCP, without
> lock_sock() protected in the context of setsockopt (already lock_sock()ed),
> and cancel unnecessary dealyed work, which is protected by lock.
> 
> [1] https://linux.die.net/man/7/tcp
> 
> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
> ---
>   net/smc/af_smc.c |  4 ++--
>   net/smc/smc_tx.c | 25 +++++++++++++++----------
>   net/smc/smc_tx.h |  1 +
>   3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index ffab9cee747d..ef021ec6b361 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -2600,8 +2600,8 @@ static int smc_setsockopt(struct socket *sock, int level, int optname,
>   		    sk->sk_state != SMC_CLOSED) {
>   			if (!val) {
>   				SMC_STAT_INC(smc, cork_cnt);
> -				mod_delayed_work(smc->conn.lgr->tx_wq,
> -						 &smc->conn.tx_work, 0);
> +				smc_tx_pending(&smc->conn);
> +				cancel_delayed_work(&smc->conn.tx_work);
>   			}
>   		}
>   		break;
> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index be241d53020f..7b0b6e24582f 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -597,27 +597,32 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
>   	return rc;
>   }
>   
> -/* Wakeup sndbuf consumers from process context
> - * since there is more data to transmit
> - */
> -void smc_tx_work(struct work_struct *work)
> +void smc_tx_pending(struct smc_connection *conn)

Could you add a comment that we're expecting lock_sock() to be held when calling 
this function?

Thanks,
Stefan