[PATCH] tcp: cleanup timer creation

[PATCH] tcp: cleanup timer creation

[Laurent Vivier]

Refactor tcp_timer_ctl() to use the epoll_add() helper instead of
manually constructing epoll_event and calling epoll_ctl() directly.

Also separate the error handling for timerfd_create() failure (-1)
from fd overflow (> FD_REF_MAX) to provide more specific debug
messages. The overflow case now logs the actual fd value, using
flow_dbg_perror() in this case was not correct (no errno set).

Delay setting conn->timer until epoll_add() succeeds, removing
redundant conn->timer = -1 assignments in error paths since the
timer is already -1 when we enter this block.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 tcp.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/tcp.c b/tcp.c
index e7fa85f346b2..58e5064db6b6 100644
--- a/tcp.c
+++ b/tcp.c
@@ -581,30 +581,32 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
 		return;
 
 	if (conn->timer == -1) {
-		union epoll_ref ref = { .type = EPOLL_TYPE_TCP_TIMER,
-					.flow = FLOW_IDX(conn) };
-		struct epoll_event ev = { .data.u64 = ref.u64,
-					  .events = EPOLLIN | EPOLLET };
-		int epollfd = flow_epollfd(&conn->f);
+		union epoll_ref ref;
 		int fd;
 
 		fd = timerfd_create(CLOCK_MONOTONIC, 0);
-		if (fd == -1 || fd > FD_REF_MAX) {
+		if (fd == -1) {
 			flow_dbg_perror(conn, "failed to get timer");
-			if (fd > -1)
-				close(fd);
-			conn->timer = -1;
 			return;
 		}
-		conn->timer = fd;
-		ref.fd = conn->timer;
+		if (fd > FD_REF_MAX) {
+			flow_dbg(conn, "timer fd overflow (%d > %d)",
+				 fd, FD_REF_MAX);
+			close(fd);
+			return;
+		}
 
-		if (epoll_ctl(epollfd, EPOLL_CTL_ADD, conn->timer, &ev)) {
-			flow_dbg_perror(conn, "failed to add timer");
-			close(conn->timer);
-			conn->timer = -1;
+		ref.type = EPOLL_TYPE_TCP_TIMER;
+		ref.flow = FLOW_IDX(conn);
+		ref.fd = fd;
+		if (epoll_add(flow_epollfd(&conn->f), EPOLLIN | EPOLLET,
+			      ref) < 0) {
+			flow_dbg(conn, "failed to add timer");
+			close(fd);
 			return;
 		}
+
+		conn->timer = fd;
 	}
 
 	if (conn->flags & ACK_TO_TAP_DUE) {
-- 
2.52.0

Re: [PATCH] tcp: cleanup timer creation

[David Gibson]

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

On Fri, Jan 09, 2026 at 12:20:34PM +0100, Laurent Vivier wrote:
> Refactor tcp_timer_ctl() to use the epoll_add() helper instead of
> manually constructing epoll_event and calling epoll_ctl() directly.
> 
> Also separate the error handling for timerfd_create() failure (-1)
> from fd overflow (> FD_REF_MAX) to provide more specific debug
> messages. The overflow case now logs the actual fd value, using
> flow_dbg_perror() in this case was not correct (no errno set).
> 
> Delay setting conn->timer until epoll_add() succeeds, removing
> redundant conn->timer = -1 assignments in error paths since the
> timer is already -1 when we enter this block.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index e7fa85f346b2..58e5064db6b6 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -581,30 +581,32 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  		return;
>  
>  	if (conn->timer == -1) {
> -		union epoll_ref ref = { .type = EPOLL_TYPE_TCP_TIMER,
> -					.flow = FLOW_IDX(conn) };
> -		struct epoll_event ev = { .data.u64 = ref.u64,
> -					  .events = EPOLLIN | EPOLLET };
> -		int epollfd = flow_epollfd(&conn->f);
> +		union epoll_ref ref;
>  		int fd;
>  
>  		fd = timerfd_create(CLOCK_MONOTONIC, 0);
> -		if (fd == -1 || fd > FD_REF_MAX) {
> +		if (fd == -1) {
>  			flow_dbg_perror(conn, "failed to get timer");
> -			if (fd > -1)
> -				close(fd);
> -			conn->timer = -1;
>  			return;
>  		}
> -		conn->timer = fd;
> -		ref.fd = conn->timer;
> +		if (fd > FD_REF_MAX) {
> +			flow_dbg(conn, "timer fd overflow (%d > %d)",
> +				 fd, FD_REF_MAX);
> +			close(fd);
> +			return;
> +		}
>  
> -		if (epoll_ctl(epollfd, EPOLL_CTL_ADD, conn->timer, &ev)) {
> -			flow_dbg_perror(conn, "failed to add timer");
> -			close(conn->timer);
> -			conn->timer = -1;
> +		ref.type = EPOLL_TYPE_TCP_TIMER;
> +		ref.flow = FLOW_IDX(conn);
> +		ref.fd = fd;
> +		if (epoll_add(flow_epollfd(&conn->f), EPOLLIN | EPOLLET,
> +			      ref) < 0) {
> +			flow_dbg(conn, "failed to add timer");
> +			close(fd);
>  			return;
>  		}
> +
> +		conn->timer = fd;
>  	}
>  
>  	if (conn->flags & ACK_TO_TAP_DUE) {
> -- 
> 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 --]