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

On Tue, Jun 16, 2026 at 02:51:30PM +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 qpair field. This makes the separate epollid field in flow_common redundant.

Replace epoll_id_to_fd[] with qpair_to_fd[], remove flow_epollid_set(), flow_epollid_register(), flow_qp()/FLOW_QP(), and the epollid field from flow_common. FLOW_QPAIR_INVALID is no longer needed: newly allocated flows get qpair 0 from memset.

It's not obvious to me why it was needed before, but not now.

For new flows, FLOW_SETQP() sets the queue pair during creation, and the socket is registered with epoll separately via flow_epoll_set().

For existing flows that may move between queue pairs, add flow_migrate()/FLOW_MIGRATE(), which removes the socket from the old epoll instance and re-registers it on the new one.

TCP timers are migrated lazily: tcp_timer_handler() detects a qpair mismatch when a timer fires, moves the timerfd to the correct epoll instance, and returns without further processing.

flow_init() now takes the execution context to initialise qpair_to_fd[].

Signed-off-by: Laurent Vivier --- flow.c | 63 ++++++++++++++++++++-------------------------------- flow.h | 25 ++++++++------------- icmp.c | 3 +-- passt.c | 3 +-- tcp.c | 42 +++++++++++++++++++++++++---------- tcp_splice.c | 1 - udp_flow.c | 4 +--- 7 files changed, 66 insertions(+), 75 deletions(-)

diff --git a/flow.c b/flow.c index bf855fe0dfaf..787a7139cfc1 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];

/* Hash table to index it */ #define FLOW_HASH_LOAD 70 /* % */ @@ -362,19 +362,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]; }

/** @@ -404,40 +392,31 @@ int flow_epoll_set(const struct flow_common *f, int command, uint32_t events, }

/** - * flow_epollid_register() - Initialize the epoll id -> fd mapping - * @epollid: epoll id to associate to - * @epollfd: epoll file descriptor for this epoll id + * flow_setqp() - Set queue pair assignment for a flow + * @f: Flow to update + * @qpair: Queue pair number to assign */ -void flow_epollid_register(int epollid, int epollfd) +void flow_setqp(struct flow_common *f, unsigned int qpair) { - assert(epollid < EPOLLFD_ID_SIZE); + assert(qpair < FLOW_QPAIR_SIZE); - epoll_id_to_fd[epollid] = epollfd; -} + flow_trace((union flow *)f, "setting queue pair to %d", qpair);

-/** - * 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) -{ - if (f == NULL || f->qpair == FLOW_QPAIR_INVALID) - return QPAIR_DEFAULT; - return f->qpair; + f->qpair = qpair; }

/** - * flow_setqp() - Set queue pair assignment for a flow + * flow_migrate() - Migrate a flow to a different queue pair * @f: Flow to update * @qpair: Queue pair number to assign + * @events: epoll events to watch for + * @fd: File descriptor to register + * @sidei: Side index of the flow */ -void flow_setqp(struct flow_common *f, unsigned int qpair) +void flow_migrate(struct flow_common *f, unsigned int qpair, uint32_t events, + int fd, unsigned int sidei) { - assert(qpair < FLOW_QPAIR_MAX); + assert(qpair < FLOW_QPAIR_SIZE);

if (f->qpair == qpair) return; @@ -445,7 +424,10 @@ void flow_setqp(struct flow_common *f, unsigned int qpair) flow_trace((union flow *)f, "updating queue pair from %d to %d", f->qpair, qpair);

+ epoll_del(qpair_to_fd[f->qpair], fd); + f->qpair = qpair; + flow_epoll_set(f, EPOLL_CTL_ADD, events, fd, sidei); }

/** @@ -669,7 +651,6 @@ union flow *flow_alloc(void)

flow_new_entry = flow; memset(flow, 0, sizeof(*flow)); - flow->f.qpair = FLOW_QPAIR_INVALID; flow_set_state(&flow->f, FLOW_STATE_NEW);

return flow; @@ -1295,8 +1276,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;

@@ -1306,4 +1288,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 3c74dcbd95c4..53e0408a9ee5 100644 --- a/flow.h +++ b/flow.h @@ -157,6 +157,8 @@ struct flowside { in_port_t eport; };

+extern int qpair_to_fd[]; + /** * flowside_eq() - Check if two flowsides are equal * @left, @right: Flowsides to compare @@ -204,20 +206,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) -#define FLOW_QPAIR_INVALID FLOW_QPAIR_MAX - -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) @@ -273,18 +267,17 @@ 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_setqp(struct flow_common *f, unsigned int qpair); #define FLOW_SETQP(flow_, _qpair) \ (flow_setqp(&(flow_)->f, _qpair)) +void flow_migrate(struct flow_common *f, unsigned int qpair, uint32_t events, + int fd, unsigned int sidei); +#define FLOW_MIGRATE(flow_, qpair_, events_, fd_, sidei_) \ + (flow_migrate(&(flow_)->f, qpair_, events_, fd_, sidei_))

void flow_defer_handler(const struct ctx *c, const struct timespec *now, unsigned int qpair); diff --git a/icmp.c b/icmp.c index 2558fe5beaab..98ce55a8aff0 100644 --- a/icmp.c +++ b/icmp.c @@ -216,7 +216,6 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c, goto cancel;

FLOW_SETQP(pingf, qpair);

Having both FLOW_SETQP() and FLOW_MIGRATE() seems dangerous to me. I think setting the initial value in flow_initiate_() or similar, then only changing it via FLOW_MIGRATE() seems like a better idea.

- 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); @@ -307,7 +306,7 @@ int icmp_tap_handler(const struct ctx *c, unsigned int qpair, uint8_t pif,

if (flow) { pingf = &flow->ping; - FLOW_SETQP(pingf, qpair); /* XXX if qpair change, update epollfd */ + FLOW_MIGRATE(pingf, qpair, EPOLLIN, pingf->sock, TGTSIDE); } else if (!(pingf = icmp_ping_new(c, qpair, af, id, saddr, daddr))) { return 1; } diff --git a/passt.c b/passt.c index c9e456641e85..3afc59b19120 100644 --- a/passt.c +++ b/passt.c @@ -365,7 +365,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"); @@ -388,7 +387,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 c0a4de33f068..1549e14adaf4 100644 --- a/tcp.c +++ b/tcp.c @@ -541,6 +541,21 @@ static int tcp_epoll_ctl(struct tcp_tap_conn *conn) return 0; }

+static int tcp_timer_epoll_add(struct tcp_tap_conn *conn, int fd) +{ + union epoll_ref ref; + + 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"); + return -1; + } + + return 0; +} + /** * tcp_timer_ctl() - Set timerfd based on flags/events, create timerfd if needed * @c: Execution context @@ -555,7 +570,6 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) return;

if (conn->timer == -1) { - union epoll_ref ref; int fd;

fd = timerfd_create(CLOCK_MONOTONIC, 0); @@ -570,12 +584,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) return; }

- 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"); + if (tcp_timer_epoll_add(conn, fd) < 0) { close(fd); return; } @@ -1736,7 +1745,6 @@ static void tcp_conn_from_tap(const struct ctx *c, unsigned int qpair, conn->sock = s; conn->timer = -1; FLOW_SETQP(conn, qpair); - flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, TGTSIDE) < 0) { flow_perror(flow, "Can't register with epoll"); goto cancel; @@ -2315,8 +2323,9 @@ int tcp_tap_handler(const struct ctx *c, unsigned int qpair, uint8_t pif, assert(pif_at_sidx(sidx) == PIF_TAP); conn = &flow->tcp;

- /* update queue pair */ - FLOW_SETQP(flow, qpair); + FLOW_MIGRATE(flow, qpair, + tcp_conn_epoll_events(conn->events, conn->flags), + conn->sock, !TAPSIDE(conn));

Doesn't the timer fd also need to be migrated to the matching qpair/epollfd?

flow_trace(conn, "packet length %zu from tap", l4len);

@@ -2523,7 +2532,6 @@ static void tcp_tap_conn_from_sock(const struct ctx *c, union flow *flow, conn->ws_to_tap = conn->ws_from_tap = 0;

FLOW_SETQP(conn, QPAIR_DEFAULT); - flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, INISIDE) < 0) { flow_perror(flow, "Can't register with epoll"); conn_flag(c, conn, CLOSING); @@ -2646,6 +2654,17 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref, assert(!c->no_tcp); assert(conn->f.type == FLOW_TCP);

+ if (conn->f.qpair != qpair) { + int old_epollfd = qpair_to_fd[qpair]; + + epoll_del(old_epollfd, conn->timer); + if (tcp_timer_epoll_add(conn, conn->timer) < 0) { + close(conn->timer); + conn->timer = -1; + }

Oh, I see, it's done lazily. Why? Also, don't you still need to actually process the current timer event as well as migrate it for future events?

+ return; + } + /* We don't reset timers on ~ACK_FROM_TAP_DUE, ~ACK_TO_TAP_DUE. If the * timer is currently armed, this event came from a previous setting, * and we just set the timer to a new point in the future: discard it. @@ -3853,7 +3872,6 @@ int tcp_flow_migrate_target(struct ctx *c, int fd) }

FLOW_SETQP(conn, QPAIR_DEFAULT); - 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 1a77ac2e8a18..3215337d3e62 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -378,7 +378,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_SETQP(conn, QPAIR_DEFAULT); - 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 44e0c4c50ca9..27dc24ffb2ae 100644 --- a/udp_flow.c +++ b/udp_flow.c @@ -154,7 +154,6 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, unsigned int qpair, uflow->activity[INISIDE] = 1; uflow->activity[TGTSIDE] = 0; FLOW_SETQP(uflow, qpair); - flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT);

flow_foreach_sidei(sidei) { if (pif_is_socket(uflow->f.pif[sidei])) @@ -292,8 +291,7 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c, unsigned int qpair, srcport, dstport); if ((uflow = udp_at_sidx(sidx))) { udp_flow_activity(uflow, sidx.sidei, now); - /* update qpair */ - FLOW_SETQP(uflow, qpair); /* if qpair changes, update epollfd */ + FLOW_MIGRATE(uflow, qpair, EPOLLIN, uflow->s[TGTSIDE], TGTSIDE); return flow_sidx_opposite(sidx); }

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