Re: [PATCH 3/3] flow: Remove EPOLLFD_ID_INVALID

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

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

On Fri, Jan 16, 2026 at 04:52:23PM +0100, Laurent Vivier wrote:
> As all flows are now registered with an epollid at creation, we no
> longer need to test if a flow is in epoll.  Remove all related code
> including flow_in_epoll() and flow_epollid_clear().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

LGTM, except for one possible nit.

> ---
>  flow.c     | 35 ++---------------------------------
>  flow.h     |  6 +-----
>  icmp.c     |  1 -
>  tcp.c      |  1 -
>  udp_flow.c |  1 -
>  5 files changed, 3 insertions(+), 41 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index 532339ce7fe1..0c5503164714 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -127,7 +127,7 @@ static_assert(ARRAY_SIZE(flow_epoll) == FLOW_NUM_TYPES,
>  unsigned flow_first_free;
>  union flow flowtab[FLOW_MAX];
>  static const union flow *flow_new_entry; /* = NULL */
> -static int epoll_id_to_fd[EPOLLFD_ID_MAX];
> +static int epoll_id_to_fd[EPOLLFD_ID_SIZE];
>  
>  /* Hash table to index it */
>  #define FLOW_HASH_LOAD		70		/* % */
> @@ -351,18 +351,6 @@ static void flow_set_state(struct flow_common *f, enum flow_state state)
>  	flow_log_details_(f, LOG_DEBUG, MAX(state, oldstate));
>  }
>  
> -/**
> - * flow_in_epoll() - Check if flow is registered with an epoll instance
> - * @f:		Flow to check
> - *
> - * 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;
> -}
> -
>  /**
>   * flow_epollfd() - Get the epoll file descriptor for a flow
>   * @f:		Flow to query
> @@ -371,13 +359,6 @@ bool flow_in_epoll(const struct flow_common *f)
>   */
>  int flow_epollfd(const struct flow_common *f)
>  {
> -	if (f->epollid >= EPOLLFD_ID_MAX) {
> -		flow_log_(f, true, LOG_WARNING,
> -			  "Invalid epollid %i for flow, assuming default",
> -			  f->epollid);
> -		return epoll_id_to_fd[EPOLLFD_ID_DEFAULT];
> -	}
> -
>  	return epoll_id_to_fd[f->epollid];
>  }
>  
> @@ -388,20 +369,11 @@ int flow_epollfd(const struct flow_common *f)
>   */
>  void flow_epollid_set(struct flow_common *f, int epollid)
>  {
> -	ASSERT(epollid < EPOLLFD_ID_MAX);
> +	ASSERT(epollid < EPOLLFD_ID_SIZE);
>  
>  	f->epollid = epollid;
>  }
>  
> -/**
> - * flow_epollid_clear() - Clear the flow epoll id
> - * @f:		Flow to update
> - */
> -void flow_epollid_clear(struct flow_common *f)
> -{
> -	f->epollid = EPOLLFD_ID_INVALID;
> -}
> -
>  /**
>   * flow_epoll_set() - Add or modify epoll registration for a flow socket
>   * @f:		Flow to register socket for
> @@ -435,8 +407,6 @@ int flow_epoll_set(const struct flow_common *f, int command, uint32_t events,
>   */
>  void flow_epollid_register(int epollid, int epollfd)
>  {
> -	ASSERT(epollid < EPOLLFD_ID_MAX);
> -

As with the other ASSERT()s this one could be updated to
EPOLLFD_ID_SIZE, rather than removed.

>  	epoll_id_to_fd[epollid] = epollfd;
>  }
>  
> @@ -643,7 +613,6 @@ union flow *flow_alloc(void)
>  
>  	flow_new_entry = flow;
>  	memset(flow, 0, sizeof(*flow));
> -	flow_epollid_clear(&flow->f);
>  	flow_set_state(&flow->f, FLOW_STATE_NEW);
>  
>  	return flow;
> diff --git a/flow.h b/flow.h
> index a74e13507957..d636358df422 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -178,7 +178,7 @@ int flowside_connect(const struct ctx *c, int s,
>   * @pif[]:	Interface for each side of the flow
>   * @side[]:	Information for each side of the flow
>   * @tap_omac:	MAC address of remote endpoint as seen from the guest
> - * @epollid:	epollfd identifier, or EPOLLFD_ID_INVALID
> + * @epollid:	epollfd identifier
>   */
>  struct flow_common {
>  #ifdef __GNUC__
> @@ -203,8 +203,6 @@ struct flow_common {
>  
>  #define EPOLLFD_ID_DEFAULT	0
>  #define EPOLLFD_ID_SIZE		(1 << EPOLLFD_ID_BITS)
> -#define EPOLLFD_ID_MAX		(EPOLLFD_ID_SIZE - 1)
> -#define EPOLLFD_ID_INVALID	EPOLLFD_ID_MAX
>  
>  #define FLOW_INDEX_BITS		17	/* 128k - 1 */
>  #define FLOW_MAX		MAX_FROM_BITS(FLOW_INDEX_BITS)
> @@ -261,10 +259,8 @@ flow_sidx_t flow_lookup_sa(const struct ctx *c, uint8_t proto, uint8_t pif,
>  union flow;
>  
>  void flow_init(void);
> -bool flow_in_epoll(const struct flow_common *f);
>  int flow_epollfd(const struct flow_common *f);
>  void flow_epollid_set(struct flow_common *f, int epollid);
> -void flow_epollid_clear(struct flow_common *f);
>  int flow_epoll_set(const struct flow_common *f, int command, uint32_t events,
>  		   int fd, unsigned int sidei);
>  void flow_epollid_register(int epollid, int epollfd);
> diff --git a/icmp.c b/icmp.c
> index eb7f11be5dad..9da5a787d181 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -213,7 +213,6 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
>  	if (flow_epoll_set(&pingf->f, EPOLL_CTL_ADD, EPOLLIN, pingf->sock,
>  			   TGTSIDE) < 0) {
>  		close(pingf->sock);
> -		flow_epollid_clear(&pingf->f);
>  		goto cancel;
>  	}
>  
> diff --git a/tcp.c b/tcp.c
> index d9bca041dea8..a23e4b067b36 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -3749,7 +3749,6 @@ static int tcp_flow_repair_connect(const struct ctx *c,
>  		return rc;
>  	}
>  
> -	flow_epollid_clear(&conn->f);
>  	conn->timer = -1;
>  	conn->listening_sock = -1;
>  
> diff --git a/udp_flow.c b/udp_flow.c
> index 80b15433f0ac..d824e2c55b2c 100644
> --- a/udp_flow.c
> +++ b/udp_flow.c
> @@ -86,7 +86,6 @@ static int udp_flow_sock(const struct ctx *c,
>  	flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT);
>  	if (flow_epoll_set(&uflow->f, EPOLL_CTL_ADD, EPOLLIN, s, sidei) < 0) {
>  		rc = -errno;
> -		flow_epollid_clear(&uflow->f);
>  		close(s);
>  		return rc;
>  	}
> -- 
> 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 --]