Re: [PATCH 2/3] tcp: Register fds with epoll at flow creation

[Laurent Vivier <lvivier@redhat.com>]

On 1/19/26 05:48, David Gibson wrote:
> On Fri, Jan 16, 2026 at 04:52:22PM +0100, Laurent Vivier wrote:
>> Register connection sockets with epoll using empty events
>> (events=0) in tcp_conn_from_tap(), tcp_tap_conn_from_sock()
>> and tcp_flow_repair_socket().
>>
>> This allows tcp_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>
> 
> Couple of queries, but the concept looks good.
> 
>> ---
>>   flow.c |  1 +
>>   tcp.c  | 36 ++++++++++++++----------------------
>>   2 files changed, 15 insertions(+), 22 deletions(-)
>>
>> diff --git a/flow.c b/flow.c
>> index cefe6c8b5b24..532339ce7fe1 100644
>> --- a/flow.c
>> +++ b/flow.c
>> @@ -357,6 +357,7 @@ static void flow_set_state(struct flow_common *f, enum flow_state state)
>>    *
>>    * Return: true if flow is registered with epoll, false otherwise
>>    */
>> +/* cppcheck-suppress unusedFunction */
>>   bool flow_in_epoll(const struct flow_common *f)
>>   {
>>   	return f->epollid != EPOLLFD_ID_INVALID;
>> diff --git a/tcp.c b/tcp.c
>> index 1db861705ddb..d9bca041dea8 100644
>> --- a/tcp.c
>> +++ b/tcp.c
>> @@ -528,37 +528,22 @@ static uint32_t tcp_conn_epoll_events(uint8_t events, uint8_t conn_flags)
>>   static int tcp_epoll_ctl(struct tcp_tap_conn *conn)
>>   {
>>   	uint32_t events;
>> -	int m;
>>   
>>   	if (conn->events == CLOSED) {
>> -		if (flow_in_epoll(&conn->f)) {
>> -			int epollfd = flow_epollfd(&conn->f);
>> +		int epollfd = flow_epollfd(&conn->f);
>>   
>> -			epoll_del(epollfd, conn->sock);
>> -			if (conn->timer != -1)
>> -				epoll_del(epollfd, conn->timer);
>> -		}
>> +		epoll_del(epollfd, conn->sock);
>> +		if (conn->timer != -1)
>> +			epoll_del(epollfd, conn->timer);
>>   
>>   		return 0;
>>   	}
>>   
>>   	events = tcp_conn_epoll_events(conn->events, conn->flags);
>>   
>> -	if (flow_in_epoll(&conn->f)) {
>> -		m = EPOLL_CTL_MOD;
>> -	} else {
>> -		flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>> -		m = EPOLL_CTL_ADD;
>> -	}
>> -
>> -	if (flow_epoll_set(&conn->f, m, events, conn->sock,
>> -			   !TAPSIDE(conn)) < 0) {
>> -		int ret = -errno;
>> -
>> -		if (m == EPOLL_CTL_ADD)
>> -			flow_epollid_clear(&conn->f);
>> -		return ret;
>> -	}
>> +	if (flow_epoll_set(&conn->f, EPOLL_CTL_MOD, events, conn->sock,
>> +			   !TAPSIDE(conn)) < 0)
>> +		return -errno;
>>   
>>   	return 0;
>>   }
>> @@ -1710,6 +1695,8 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
>>   	conn->sock = s;
>>   	conn->timer = -1;
>>   	conn->listening_sock = -1;
>> +	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>> +	flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, !TAPSIDE(conn));
> 
> Do we need to handle errors here?

It's a good question. If EPOLL_CTL_ADD fails, the subsequent EPOLL_CTL_MOD called from 
tcp_epoll_ctl() will fail, so we can rely on that to handle the error... but in 
conn_event(), tcp_epoll_ctl() error is not handler.

So I think we need at least to handle the error here.

> 
> Because this is conn_from_tap(), we know that !TAPSIDE() will always
> be TGTSIDE in this case.
> 
>>   	conn_event(c, conn, TAP_SYN_RCVD);
>>   
>>   	conn->wnd_to_tap = WINDOW_DEFAULT;
>> @@ -2433,6 +2420,8 @@ static void tcp_tap_conn_from_sock(const struct ctx *c, union flow *flow,
>>   	conn->sock = s;
>>   	conn->timer = -1;
>>   	conn->ws_to_tap = conn->ws_from_tap = 0;
>> +	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>> +	flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, !TAPSIDE(conn));
> 
> Same comments as above, except here we know it's INISIDE.

Same answer.

> 
>>   	conn_event(c, conn, SOCK_ACCEPTED);
>>   
>>   	hash = flow_hash_insert(c, TAP_SIDX(conn));
>> @@ -3825,6 +3814,9 @@ int tcp_flow_migrate_target(struct ctx *c, int fd)
>>   		return 0;
>>   	}
>>   
>> +	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>> +	flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock, !TAPSIDE(conn));
>> +
>>   	flow_hash_insert(c, TAP_SIDX(conn));
>>   	FLOW_ACTIVATE(conn);
>>   
>> -- 
>> 2.52.0
>>
>