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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3225 bytes --]

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?

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]