On Thu, Jun 04, 2026 at 07:44:56AM +0200, Stefano Brivio wrote:
> 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).

Sure, will do.

-- 
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