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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YgDuX/TW0AKrbnpe@TonyMac-Alibaba>
Date:   Mon, 7 Feb 2022 18:03:11 +0800
From:   Tony Lu <tonylu@...ux.alibaba.com>
To:     Stefan Raspl <raspl@...ux.ibm.com>
Cc:     kgraul@...ux.ibm.com, kuba@...nel.org, davem@...emloft.net,
        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 Mon, Jan 31, 2022 at 08:13:53PM +0100, Stefan Raspl wrote:
> 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?

I will add it in the next separated patch.

Thank you,
Tony Lu