Re: [PATCH 8/8] tcp_splice: Exit forwarding earlier when stalled read side

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 4 Jun 2026 15:26:36 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 04, 2026 at 06:41:47AM +0200, Stefano Brivio wrote:
> > On Thu, 28 May 2026 15:02:13 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > At the end of our loop we have a conditional 'break' that exits if we're
> > > at EOF on the read side and have nothing left in the pipe.  This doesn't
> > > depend on anything write-side, so we can move it earlier, avoiding an
> > > unnecessary write side splice in this case.
> > > 
> > > Furthermore, there's also nothing to be done write side if we've hit EAGAIN
> > > on the read side and the pipe is empty, so exit early for that case as
> > > well.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  tcp_splice.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tcp_splice.c b/tcp_splice.c
> > > index 565596d3..623ca926 100644
> > > --- a/tcp_splice.c
> > > +++ b/tcp_splice.c
> > > @@ -497,9 +497,17 @@ static int tcp_splice_forward(struct ctx *c,
> > >  
> > >  		flow_trace(conn, "%zi from read-side call", readlen);
> > >  
> > > -		if (!readlen) {
> > > -			conn_event(conn, FIN_RCVD(fromsidei));
> > > -		} else if (readlen > 0) {
> > > +		if (readlen <= 0) {
> > > +			if (!readlen) /* EOF */
> > > +				conn_event(conn, FIN_RCVD(fromsidei));
> > > +
> > > +			/* We're either blocked or at EOF on the read side, and
> > > +			 * there's nothing in the pipe so there's nothing to do
> > > +			 * write side either.
> > > +			 */
> > > +			if (!conn->pending[fromsidei])
> > > +				break;
> > > +		} else {
> > >  			conn->pending[fromsidei] += readlen;
> > >  
> > >  			if (readlen >= (long)c->tcp.pipe_size * 90 / 100)
> > > @@ -530,10 +538,6 @@ static int tcp_splice_forward(struct ctx *c,
> > >  			break;
> > >  
> > >  		conn->pending[fromsidei] -= written;
> > > -
> > > -		if (conn->events & FIN_RCVD(fromsidei) &&
> > > -		    !conn->pending[fromsidei])
> > > -			break;  
> > 
> > The rest of the series looks good to me and I'm running tests now
> > before pushing, but I can't convince myself of the correctness of this
> > change.
> > 
> > The first part makes sense as an additional condition to exit the loop
> > and avoid an additional splice() call that would just return EAGAIN.
> > 
> > But this one is a different condition because it happens to check
> > conn->pending[fromsidei] right after we subtracted 'written' from it,
> > and we know we have no input data anymore, so it avoids a useless
> > (although I think harmless) read-side splice() call in the next
> > iteration of the loop, doesn't it?  
> 
> Good point.  If there was data in the pipe before the read-splice(),
> then we get EOF, then we empty the pipe with the write-splice() we
> know we're done and don't need to read-splice() again.
> 
> I'm also pretty sure it's harmless, as long as repeated read-splice()s
> after EOF return either EOF again or EAGAIN, and I can't really see it
> doing anything else.
> 
> Given that, and the fact you're already testing, I guess it makes more
> sense to do a fixup on top, rather than respinning?

But the commit message of this one would still be misleading because
we're not actually "moving" the condition, and the fix-up would just
add back these four lines (I guess?), so I think it would be more
practical if you could send a new version of 8/8 only (you can base it
on 7/8).

-- 
Stefano