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

[Laurent Vivier <lvivier@redhat.com>]

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.

Thanks,
Laurent
> 
>> +	}
>> +
>>   	conn_event(conn, SPLICE_CONNECT);
>>   
>>   	if (connect(conn->s[1], &sa.sa, socklen_inany(&sa))) {
>> -- 
>> 2.52.0
>>
>