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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 21 May 2026 12:11:49 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

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

Okay, fair. That's what I call "not really obvious" but it's probably
obvious enough.

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