Re: [PATCH] SUNRPC: Use TCP_CORK to optimise send performance on the server

[Trond Myklebust <trondmy@hammerspace.com>]

On Sun, 2021-02-14 at 17:27 +0000, Chuck Lever wrote:
> 
> 
> > On Feb 14, 2021, at 11:21 AM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > 
> > On Sat, 2021-02-13 at 23:30 +0000, Chuck Lever wrote:
> > > 
> > > 
> > > > On Feb 13, 2021, at 5:10 PM, Trond Myklebust <
> > > > trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Sat, 2021-02-13 at 21:53 +0000, Chuck Lever wrote:
> > > > > Hi Trond-
> > > > > 
> > > > > > On Feb 13, 2021, at 3:25 PM, trondmy@kernel.org wrote:
> > > > > > 
> > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > 
> > > > > > Use a counter to keep track of how many requests are queued
> > > > > > behind
> > > > > > the
> > > > > > xprt->xpt_mutex, and keep TCP_CORK set until the queue is
> > > > > > empty.
> > > > > 
> > > > > I'm intrigued, but IMO, the patch description needs to
> > > > > explain
> > > > > why this change should be made. Why abandon Nagle?
> > > > > 
> > > > 
> > > > This doesn't change the Nagle/TCP_NODELAY settings. It just
> > > > switches to
> > > > using the new documented kernel interface.
> > > > 
> > > > The only change is to use TCP_CORK so that we don't send out
> > > > partially
> > > > filled TCP frames, when we can see that there are other RPC
> > > > replies
> > > > that are queued up for transmission.
> > > > 
> > > > Note the combination TCP_CORK+TCP_NODELAY is common, and the
> > > > main
> > > > effect of the latter is that when we turn off the TCP_CORK,
> > > > then
> > > > there
> > > > is an immediate forced push of the TCP queue.
> > > 
> > > The description above suggests the patch is just a
> > > clean-up, but a forced push has potential to change
> > > the server's behavior.
> > 
> > Well, yes. That's very much the point.
> > 
> > Right now, the TCP_NODELAY/Nagle setting means that we're doing
> > that
> > forced push at the end of _every_ RPC reply, whether or not there
> > is
> > more stuff that can be queued up in the socket. The MSG_MORE is the
> > only thing that keeps us from doing the forced push on every
> > sendpage()
> > call.
> > So the TCP_CORK is there to _further delay_ that forced push until
> > we
> > think the queue is empty.
> 
> My concern is that waiting for the queue to empty before pushing
> could
> improve throughput at the cost of increased average round-trip
> latency.
> That concern is based on experience I've had attempting to batch
> sends
> in the RDMA transport.
> 
> 
> > IOW: it attempts to optimise the scheduling of that push until
> > we're
> > actually done pushing more stuff into the socket.
> 
> Yep, clear, thanks. It would help a lot if the above were included in
> the patch description.
> 
> And, I presume that the TCP layer will push anyway if it needs to
> reclaim resources to handle more queued sends.
> 
> Let's also consider starvation; ie, that the server will continue
> queuing replies such that it never uncorks. The logic in the patch
> appears to depend on the client stopping at some point to wait for
> the
> server to catch up. There probably should be a trap door that uncorks
> after a few requests (say, 8) or certain number of bytes are pending
> without a break.

So, firstly, the TCP layer will still push every time a frame is full,
so traffic does not stop altogether while TCP_CORK is set. Secondly,
TCP will also always push on send errors (e.g. when out of free socket
buffer). Thirdly, it will always push after hitting the 200ms ceiling,
as described in the tcp(7) manpage.

IOW: The TCP_CORK feature is not designed to block the socket forever.
It is there to allow the application to hint to the TCP layer what it
needs to do in exactly the same way that MSG_MORE does.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com