netdev - Re: [PATCH net-next 1/3] net/smc: Send directly when TCP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74aaa8ce-81a4-b048-cee2-b137279d13d5@linux.ibm.com>
Date:   Mon, 31 Jan 2022 20:13:53 +0100
From:   Stefan Raspl <raspl@...ux.ibm.com>
To:     Tony Lu <tonylu@...ux.alibaba.com>, kgraul@...ux.ibm.com,
        kuba@...nel.org, davem@...emloft.net
Cc:     netdev@...r.kernel.org, linux-s390@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net/smc: Send directly when TCP_CORK is
 cleared

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@...ux.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