Re: [PATCH 6/6] tcp_splice: Simplify shutdown(2) handling

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

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

On Wed, May 20, 2026 at 10:30:23PM +0200, Stefano Brivio wrote:
> On Wed, 20 May 2026 23:08:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At the end of tcp_splice_forward(), we check for half-closed connections
> > and propagate the FIN to the other side with a shutdown(2).  Currently we
> > check for a half closed connection in either direction.  That's unnecessary
> > here, because tcp_splice_forward() will already be called for each
> > direction if there are any relevant events.
> 
> True, but do we have the guarantee that tcp_splice_forward() will also
> be called once all relevant FIN_RCVD / FIN_SENT flags have been sent?

Yes, because tcp_splice_forward() is (now) the only place that *sets*
FIN_RCVD (or FIN_SENT).

> The reason why we check both sides here is that we might have updated
> flags for one side, and now we need to double check if it's time to
> call shutdown() as a consequence.
> 
> Maybe we never have to, but I think it's not really obvious to prove.

tcp_splice_forward() only touches FIN_RCVD(fromsidei).  So, we only
need to examine FIN_RCVD(fromsidei).  If FIN_RCVD is set for the other
side, it must be in another call to tcp_splice_forward() which will
also examine that other flag and shutdown() as necessary.

> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp_splice.c | 22 ++++++++--------------
> >  1 file changed, 8 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tcp_splice.c b/tcp_splice.c
> > index b45f0060..e5018f2e 100644
> > --- a/tcp_splice.c
> > +++ b/tcp_splice.c
> > @@ -582,21 +582,15 @@ retry:
> >  			break;
> >  	}
> >  
> > -	if (!conn->pending[fromsidei] &&
> > -	    conn->events & FIN_RCVD(fromsidei)) {
> > -		unsigned sidei;
> > -
> > -		flow_foreach_sidei(sidei) {
> > -			if ((conn->events & FIN_RCVD(sidei)) &&
> > -			    !(conn->events & FIN_SENT(!sidei))) {
> > -				if (shutdown(conn->s[!sidei], SHUT_WR) < 0) {
> > -					flow_perror(conn, "shutdown() on %s",
> > -						    pif_name(conn->f.pif[!sidei]));
> > -					return -1;
> > -				}
> > -				conn_event(conn, FIN_SENT(!sidei));
> > -			}
> > +	if ((conn->events & FIN_RCVD(fromsidei)) &&
> > +	    !(conn->events & FIN_SENT(!fromsidei)) &&
> > +	    !conn->pending[fromsidei]) {
> > +		if (shutdown(conn->s[!fromsidei], SHUT_WR) < 0) {
> > +			flow_perror(conn, "shutdown() on %s",
> > +				    pif_name(conn->f.pif[!fromsidei]));
> > +			return -1;
> >  		}
> > +		conn_event(conn, FIN_SENT(!fromsidei));
> >  	}
> >  
> >  	return 0;
> 
> -- 
> Stefano
> 

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