Re: [PATCH v6 12/12] flow: Derive epoll fd from queue pair, removing epollid field

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

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

On Fri, Jul 31, 2026 at 06:16:17PM +0200, Laurent Vivier wrote:
> Since each queue pair maps to exactly one epoll instance, the epoll
> file descriptor can be looked up directly from the flow's qpair
> field.  This makes the separate epollid field and its indirection
> through epoll_id_to_fd[] redundant.
> 
> Replace epoll_id_to_fd[] with qpair_to_fd[], derived from the qpair
> assigned at flow creation.  Remove flow_epollid_set(),
> flow_epollid_register(), flow_qp()/FLOW_QP(), and the epollid field
> from flow_common.  flow_epollfd() now simply returns
> qpair_to_fd[f->qpair].
> 
> flow_init() now takes the execution context to initialise
> qpair_to_fd[] from c->epollfd.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

A few minor comments, otherwise LGTM.

> ---
>  flow.c       | 55 +++++++++-------------------------------------------
>  flow.h       | 20 +++++--------------
>  icmp.c       |  1 -
>  passt.c      |  3 +--
>  tcp.c        |  3 ---
>  tcp_splice.c |  1 -
>  udp_flow.c   |  1 -
>  7 files changed, 15 insertions(+), 69 deletions(-)
> 
> diff --git a/flow.c b/flow.c
> index 40b2e68be0ab..1efa9b966991 100644
> --- a/flow.c
> +++ b/flow.c
> @@ -130,7 +130,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_SIZE];
> +int qpair_to_fd[FLOW_QPAIR_SIZE];

Nit: I'd suggest qpair_to_epollfd[], since there could bve a bunch of
fds related to a qpair one way or another.

>  
>  /* Hash table to index it */
>  #define FLOW_HASH_LOAD		70		/* % */
> @@ -364,19 +364,7 @@ static void flow_set_state(struct flow_common *f, enum flow_state state)
>   */
>  int flow_epollfd(const struct flow_common *f)
>  {
> -	return epoll_id_to_fd[f->epollid];
> -}
> -
> -/**
> - * flow_epollid_set() - Associate a flow with an epoll id
> - * @f:		Flow to update
> - * @epollid:	epoll id to associate with this flow
> - */
> -void flow_epollid_set(struct flow_common *f, int epollid)
> -{
> -	assert(epollid < EPOLLFD_ID_SIZE);
> -
> -	f->epollid = epollid;
> +	return qpair_to_fd[f->qpair];
>  }
>  
>  /**
> @@ -405,31 +393,6 @@ int flow_epoll_set(const struct flow_common *f, int command, uint32_t events,
>  	return epoll_ctl(flow_epollfd(f), command, fd, &ev);
>  }
>  
> -/**
> - * flow_epollid_register() - Initialize the epoll id -> fd mapping
> - * @epollid:	epoll id to associate to
> - * @epollfd:	epoll file descriptor for this epoll id
> - */
> -void flow_epollid_register(int epollid, int epollfd)
> -{
> -	assert(epollid < EPOLLFD_ID_SIZE);
> -
> -	epoll_id_to_fd[epollid] = epollfd;
> -}
> -
> -/**
> - * flow_qp() - Get the queue pair for a flow
> - * @f:		Flow to query (may be NULL)
> - *
> - * Return: queue pair number for the flow, or 0 if flow is NULL or has no
> - *         valid queue pair assignment
> - */
> -/* cppcheck-suppress unusedFunction */
> -unsigned int flow_qp(const struct flow_common *f)
> -{
> -	return f->qpair;
> -}
> -
>  /**
>   * flow_setqp() - Set queue pair assignment for a flow
>   * @f:		Flow to update
> @@ -437,13 +400,9 @@ unsigned int flow_qp(const struct flow_common *f)
>   */
>  static void flow_setqp(struct flow_common *f, unsigned int qpair)
>  {
> -	assert(qpair < FLOW_QPAIR_MAX);
> -
> -	if (f->qpair == qpair)
> -		return;
> +	assert(qpair < FLOW_QPAIR_SIZE);
>  
> -	flow_trace((union flow *)f, "updating queue pair from %d to %d",
> -		   f->qpair, qpair);
> +	flow_trace((union flow *)f, "setting queue pair to %d", qpair);
>  
>  	f->qpair = qpair;
>  }
> @@ -1299,8 +1258,9 @@ int flow_migrate_target(struct ctx *c, const struct migrate_stage *stage,
>  
>  /**
>   * flow_init() - Initialise flow related data structures
> + * @c:		Execution context
>   */
> -void flow_init(void)
> +void flow_init(const struct ctx *c)
>  {
>  	unsigned b;
>  
> @@ -1310,4 +1270,7 @@ void flow_init(void)
>  
>  	for (b = 0; b < FLOW_HASH_SIZE; b++)
>  		flow_hashtab[b] = FLOW_SIDX_NONE;
> +
> +	for (b = 0; b < FLOW_QPAIR_SIZE; b++)
> +		qpair_to_fd[b] = c->epollfd;
>  }
> diff --git a/flow.h b/flow.h
> index b6f980b4f826..028d42bccc0c 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -157,6 +157,8 @@ struct flowside {
>  	in_port_t		eport;
>  };
>  
> +extern int qpair_to_fd[];

Does this need to be extern?  Looks like everything outside flow.h
accesses it indirectly via flow_epollfd()?

> +
>  /**
>   * flowside_eq() - Check if two flowsides are equal
>   * @left, @right:	Flowsides to compare
> @@ -203,19 +205,12 @@ struct flow_common {
>  
>  	uint8_t		tap_omac[6];
>  
> -#define EPOLLFD_ID_BITS 8
> -	unsigned int	epollid:EPOLLFD_ID_BITS;
>  #define FLOW_QPAIR_BITS 5
>  	unsigned int	qpair:FLOW_QPAIR_BITS;
>  };
>  
> -#define EPOLLFD_ID_DEFAULT	0
> -#define EPOLLFD_ID_SIZE		(1 << EPOLLFD_ID_BITS)
> -
> -#define FLOW_QPAIR_NUM		(1 << FLOW_QPAIR_BITS)
> -#define FLOW_QPAIR_MAX		(FLOW_QPAIR_NUM - 1)
> -
> -static_assert(VHOST_USER_MAX_VQS <= FLOW_QPAIR_MAX * 2);
> +#define FLOW_QPAIR_SIZE		(1 << FLOW_QPAIR_BITS)
> +static_assert(VHOST_USER_MAX_VQS <= FLOW_QPAIR_SIZE * 2);
>  
>  #define FLOW_INDEX_BITS		17	/* 128k - 1 */
>  #define FLOW_MAX		MAX_FROM_BITS(FLOW_INDEX_BITS)
> @@ -271,15 +266,10 @@ flow_sidx_t flow_lookup_sa(const struct ctx *c, uint8_t proto, uint8_t pif,
>  
>  union flow;
>  
> -void flow_init(void);
> +void flow_init(const struct ctx *c);
>  int flow_epollfd(const struct flow_common *f);
> -void flow_epollid_set(struct flow_common *f, int epollid);
>  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);
> -unsigned int flow_qp(const struct flow_common *f);
> -#define FLOW_QP(flow_)				\
> -	(flow_qp(&(flow_)->f))
>  
>  void flow_defer_handler(const struct ctx *c, const struct timespec *now,
>  			unsigned int qpair);
> diff --git a/icmp.c b/icmp.c
> index e75d66d168be..46c5d925600a 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -219,7 +219,6 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
>  	if (pingf->sock > FD_REF_MAX)
>  		goto cancel;
>  
> -	flow_epollid_set(&pingf->f, EPOLLFD_ID_DEFAULT);
>  	if (flow_epoll_set(&pingf->f, EPOLL_CTL_ADD, EPOLLIN, pingf->sock,
>  			   TGTSIDE) < 0) {
>  		close(pingf->sock);
> diff --git a/passt.c b/passt.c
> index 55708d96fb06..bc9f1575ef8c 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -382,7 +382,6 @@ int main(int argc, char **argv)
>  	c->epollfd = epoll_create1(EPOLL_CLOEXEC);
>  	if (c->epollfd == -1)
>  		die_perror("Failed to create epoll file descriptor");
> -	flow_epollid_register(EPOLLFD_ID_DEFAULT, c->epollfd);
>  
>  	if (getrlimit(RLIMIT_NOFILE, &limit))
>  		die_perror("Failed to get maximum value of open files limit");
> @@ -405,7 +404,7 @@ int main(int argc, char **argv)
>  	if (clock_gettime(CLOCK_MONOTONIC, &now))
>  		die_perror("Failed to get CLOCK_MONOTONIC time");
>  
> -	flow_init();
> +	flow_init(c);
>  	fwd_scan_ports_init(c);
>  
>  	if ((!c->no_udp && udp_init(c)) || (!c->no_tcp && tcp_init(c)))
> diff --git a/tcp.c b/tcp.c
> index 39f732f8bea3..e80a64e38f24 100644
> --- a/tcp.c
<> +++ b/tcp.c
> @@ -1764,7 +1764,6 @@ static void tcp_conn_from_tap(const struct ctx *c, unsigned int qpair,
>  
>  	conn->sock = s;
>  	conn->timer = -1;
> -	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>  	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, TGTSIDE) < 0) {
>  		flow_perror_ratelimit(flow, now, "Can't register with epoll");
>  		goto cancel;
> @@ -2578,7 +2577,6 @@ static void tcp_tap_conn_from_sock(const struct ctx *c, union flow *flow,
>  	conn->timer = -1;
>  	conn->ws_to_tap = conn->ws_from_tap = 0;
>  
> -	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>  	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, INISIDE) < 0) {
>  		flow_perror_ratelimit(flow, now, "Can't register with epoll");
>  		conn_flag(c, conn, CLOSING, now);
> @@ -3876,7 +3874,6 @@ int tcp_flow_migrate_target(struct ctx *c, int fd)
>  		goto out;
>  	}
>  
> -	flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT);
>  	if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock,
>  			   !TAPSIDE(conn)))
>  		goto out; /* tcp_flow_migrate_target_ext() will clean this up */
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 4b01f1aa3602..8617e62cea9a 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -387,7 +387,6 @@ 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;
> diff --git a/udp_flow.c b/udp_flow.c
> index 2578d640a3da..8e985df8398d 100644
> --- a/udp_flow.c
> +++ b/udp_flow.c
> @@ -154,7 +154,6 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
>  	uflow->ttl[INISIDE] = uflow->ttl[TGTSIDE] = 0;
>  	uflow->activity[INISIDE] = 1;
>  	uflow->activity[TGTSIDE] = 0;
> -	flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT);
>  
>  	flow_foreach_sidei(sidei) {
>  		if (pif_is_socket(uflow->f.pif[sidei]))
> -- 
> 2.54.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 --]