netdev - Re: [PATCH net-next 2/3] net/smc: Remove corked dealyed work

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: <YgYn6jA0i3pFXoCS@TonyMac-Alibaba>
Date:   Fri, 11 Feb 2022 17:10:02 +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 2/3] net/smc: Remove corked dealyed work

On Mon, Jan 31, 2022 at 08:40:47PM +0100, Stefan Raspl wrote:
> On 1/30/22 19:02, Tony Lu wrote:
> > Based on the manual of TCP_CORK [1] and MSG_MORE [2], these two options
> > have the same effect. Applications can set these options and informs the
> > kernel to pend the data, and send them out only when the socket or
> > syscall does not specify this flag. In other words, there's no need to
> > send data out by a delayed work, which will queue a lot of work.
> > 
> > This removes corked delayed work with SMC_TX_CORK_DELAY (250ms), and the
> > applications control how/when to send them out. It improves the
> > performance for sendfile and throughput, and remove unnecessary race of
> > lock_sock(). This also unlocks the limitation of sndbuf, and try to fill
> > it up before sending.
> > 
> > [1] https://linux.die.net/man/7/tcp
> > [2] https://man7.org/linux/man-pages/man2/send.2.html
> > 
> > Signed-off-by: Tony Lu <tonylu@...ux.alibaba.com>
> > ---
> >   net/smc/smc_tx.c | 15 ++++++---------
> >   1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> > index 7b0b6e24582f..9cec62cae7cb 100644
> > --- a/net/smc/smc_tx.c
> > +++ b/net/smc/smc_tx.c
> > @@ -31,7 +31,6 @@
> >   #include "smc_tracepoint.h"
> >   #define SMC_TX_WORK_DELAY	0
> > -#define SMC_TX_CORK_DELAY	(HZ >> 2)	/* 250 ms */
> >   /***************************** sndbuf producer *******************************/
> > @@ -237,15 +236,13 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
> >   		if ((msg->msg_flags & MSG_OOB) && !send_remaining)
> >   			conn->urg_tx_pend = true;
> >   		if ((msg->msg_flags & MSG_MORE || smc_tx_is_corked(smc)) &&
> > -		    (atomic_read(&conn->sndbuf_space) >
> > -						(conn->sndbuf_desc->len >> 1)))
> > -			/* for a corked socket defer the RDMA writes if there
> > -			 * is still sufficient sndbuf_space available
> > +		    (atomic_read(&conn->sndbuf_space)))
> > +			/* for a corked socket defer the RDMA writes if
> > +			 * sndbuf_space is still available. The applications
> > +			 * should known how/when to uncork it.
> >   			 */
> > -			queue_delayed_work(conn->lgr->tx_wq, &conn->tx_work,
> > -					   SMC_TX_CORK_DELAY);
> > -		else
> > -			smc_tx_sndbuf_nonempty(conn);
> > +			continue;
> 
> In case we just corked the final bytes in this call, wouldn't this
> 'continue' prevent us from accounting the Bytes that we just staged to be
> sent out later in the trace_smc_tx_sendmsg() call below?
> 
> > +		smc_tx_sndbuf_nonempty(conn);
> >   		trace_smc_tx_sendmsg(smc, copylen);
> 

If the application send out the final bytes in this call, the
application should also clear MSG_MORE or TCP_CORK flag, this action is
required based on the manuals [1] and [2]. So it is safe to cork the data
if flag is setted, and continue to the next loop until application
clears the flag.

[1] https://linux.die.net/man/7/tcp
[2] https://man7.org/linux/man-pages/man2/send.2.html

Thank you,
Tony Lu