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

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

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

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?

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.

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

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