Re: [PATCH 1/3] tcp_splice: Register fds with epoll at flow creation

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

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

On Mon, Jan 19, 2026 at 09:10:42AM +0100, Laurent Vivier wrote:
> On 1/19/26 05:45, David Gibson wrote:
> > On Fri, Jan 16, 2026 at 04:52:21PM +0100, Laurent Vivier wrote:
> > > Register both splice connection sockets with epoll using empty events
> > > (events=0) in tcp_splice_connect(), before initiating the connection.
> > > 
> > > This allows tcp_splice_epoll_ctl() to always use EPOLL_CTL_MOD, removing
> > > the need to check whether fds are already registered. As a result, the
> > > conditional ADD/MOD logic is no longer needed, simplifying the function.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Nice!  One query below.
> > 
> > > ---
> > >   tcp_splice.c | 20 ++++++++++----------
> > >   1 file changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/tcp_splice.c b/tcp_splice.c
> > > index a7c04ca8652a..cb81e012ee4b 100644
> > > --- a/tcp_splice.c
> > > +++ b/tcp_splice.c
> > > @@ -142,20 +142,12 @@ static uint32_t tcp_splice_conn_epoll_events(uint16_t events, unsigned sidei)
> > >   static int tcp_splice_epoll_ctl(struct tcp_splice_conn *conn)
> > >   {
> > >   	uint32_t events[2];
> > > -	int m;
> > > -
> > > -	if (flow_in_epoll(&conn->f)) {
> > > -		m = EPOLL_CTL_MOD;
> > > -	} else {
> > > -		flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
> > > -		m = EPOLL_CTL_ADD;
> > > -	}
> > >   	events[0] = tcp_splice_conn_epoll_events(conn->events, 0);
> > >   	events[1] = tcp_splice_conn_epoll_events(conn->events, 1);
> > > -	if (flow_epoll_set(&conn->f, m, events[0], conn->s[0], 0) ||
> > > -	    flow_epoll_set(&conn->f, m, events[1], conn->s[1], 1)) {
> > > +	if (flow_epoll_set(&conn->f, EPOLL_CTL_MOD, events[0], conn->s[0], 0) ||
> > > +	    flow_epoll_set(&conn->f, EPOLL_CTL_MOD, events[1], conn->s[1], 1)) {
> > >   		int ret = -errno;
> > >   		flow_perror(conn, "ERROR on epoll_ctl()");
> > >   		return ret;
> > > @@ -368,6 +360,14 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn)
> > >   	pif_sockaddr(c, &sa, tgtpif, &tgt->eaddr, tgt->eport);
> > > +	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
> > > +	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->s[0], 0) ||
> > > +	    flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->s[1], 1)) {
> > > +		int ret = -errno;
> > > +		flow_perror(conn, "Cannot register to epollfd");
> > > +		return ret;
> > 
> > Do we need to worry about rollback here, if the first one succeeds,
> > but the second one fails?
> 
> If we return an error here, tcp_splice_conn_from_sock() sets the CLOSING
> flag on the connection and conn_flag() handles the closing flag by calling
> epoll_del() for both sockets. So the cleanup path handles this case already.
> 
> None of the error cases in tcp_splice_connect() worries about rollback, so
> it's simpler to do the same.

Ok, that makes sense.  Maybe add that explanation to the commit
message?

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