From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202608 header.b=jzy8TE/C; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 5B0445A0624 for ; Mon, 03 Aug 2026 05:46:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1785728795; bh=pKOGRMA5/ez7EWoCrLWo+ovjbwnJ4uilOd660gU1etM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jzy8TE/CXkmRFvX4CcbGmHG7ewFo0hO4nG+JbCFMyp6srBmULPrbxC7jdU5CnwFA4 aAVg+GmpGfHGrXtwz1a3G/EEd3Uk2S8e3gwkvLvQT0JDTgdsc/KoQqdQti3lune+Fo 3ztsnfRttPVDfDNRPKL/GOSggii+T3icZYAABBO13DsTHgPtPGQAXiJwOlH76x87M/ 5NOOutNetk8hUGUDAic8Pl92xS5xrmnhnZj3yKt13PQPKwf40+Q/3DXp0oTXZsuKo1 VL1JsZtoT3617/GlwmM549fc5DJNoRycBb1m9TS499wTdzK2Gq2hRII/Gf7ZMqqorq LF5TJ9C03agfg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hD2fM2BJgz4wHt; Mon, 03 Aug 2026 13:46:35 +1000 (AEST) Date: Mon, 3 Aug 2026 13:40:18 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v6 11/12] flow: Add queue pair tracking to flow management Message-ID: References: <20260731161617.3550626-1-lvivier@redhat.com> <20260731161617.3550626-12-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="f33FcufbTC0BFc8K" Content-Disposition: inline In-Reply-To: <20260731161617.3550626-12-lvivier@redhat.com> Message-ID-Hash: 3NQYQILUYYD3ELZBOBL753SV5ECZQIYC X-Message-ID-Hash: 3NQYQILUYYD3ELZBOBL753SV5ECZQIYC X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --f33FcufbTC0BFc8K Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 31, 2026 at 06:16:16PM +0200, Laurent Vivier wrote: > Add a qpair field to struct flow_common so each flow knows which > queue pair (and thus which worker thread) owns it. >=20 > flow_alloc() takes a qpair parameter, set at creation time. All > protocol handlers pass their qpair when allocating flows from tap, > and use QPAIR_DEFAULT for flows initiated from the host side. >=20 > tcp_keepalive() and tcp_inactivity() filter by qpair so each > worker only processes its own flows. >=20 > Signed-off-by: Laurent Vivier > --- > flow.c | 34 +++++++++++++++++++++++++++++++++- > flow.h | 13 ++++++++++++- > flow_table.h | 2 +- > icmp.c | 3 +-- > tcp.c | 20 +++++++++++++------- > udp_flow.c | 9 ++++----- > 6 files changed, 64 insertions(+), 17 deletions(-) >=20 > diff --git a/flow.c b/flow.c > index c63491804709..40b2e68be0ab 100644 > --- a/flow.c > +++ b/flow.c > @@ -417,6 +417,37 @@ void flow_epollid_register(int epollid, int epollfd) > epoll_id_to_fd[epollid] =3D epollfd; > } > =20 > +/** > + * 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 This implies it's safe to call on NULL, which doesn't appear to be the case. 0 also seems a dubious value for "no valid queue pair assignment", since it is a valid qpair number. > + * 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 > + * @qpair: Queue pair number to assign > + */ > +static void flow_setqp(struct flow_common *f, unsigned int qpair) > +{ > + assert(qpair < FLOW_QPAIR_MAX); > + > + if (f->qpair =3D=3D qpair) > + return; > + > + flow_trace((union flow *)f, "updating queue pair from %d to %d", > + f->qpair, qpair); > + > + f->qpair =3D qpair; > +} > + > /** > * flow_initiate_() - Move flow to INI, setting pif[INISIDE] > * @flow: Flow to change state > @@ -603,7 +634,7 @@ void flow_activate(struct flow_common *f) > * > * Return: pointer to an unused flow entry, or NULL if the table is full > */ > -union flow *flow_alloc(void) > +union flow *flow_alloc(unsigned int qpair) Comment for new parameter? > { > union flow *flow =3D &flowtab[flow_first_free]; > =20 > @@ -637,6 +668,7 @@ union flow *flow_alloc(void) > =20 > flow_new_entry =3D flow; > memset(flow, 0, sizeof(*flow)); > + flow_setqp(&flow->f, qpair); > flow_set_state(&flow->f, FLOW_STATE_NEW); > =20 > return flow; > diff --git a/flow.h b/flow.h > index 8d85e27427db..b6f980b4f826 100644 > --- a/flow.h > +++ b/flow.h > @@ -184,7 +184,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 > + * @qpair: Queue pair number assigned to this flow In the comments, @epollfd is replaced with @qpair, but in the structure, @qpair is added as well as @epollfd. Did you mean to leave the comment, or remove the field? > */ > struct flow_common { > #ifdef __GNUC__ > @@ -205,11 +205,18 @@ struct flow_common { > =20 > #define EPOLLFD_ID_BITS 8 > unsigned int epollid:EPOLLFD_ID_BITS; > +#define FLOW_QPAIR_BITS 5 > + unsigned int qpair:FLOW_QPAIR_BITS; > }; > =20 > #define EPOLLFD_ID_DEFAULT 0 > #define EPOLLFD_ID_SIZE (1 << EPOLLFD_ID_BITS) > =20 > +#define FLOW_QPAIR_NUM (1 << FLOW_QPAIR_BITS) > +#define FLOW_QPAIR_MAX (FLOW_QPAIR_NUM - 1) > + > +static_assert(VHOST_USER_MAX_VQS <=3D FLOW_QPAIR_MAX * 2); > + > #define FLOW_INDEX_BITS 17 /* 128k - 1 */ > #define FLOW_MAX MAX_FROM_BITS(FLOW_INDEX_BITS) > =20 > @@ -270,6 +277,10 @@ void flow_epollid_set(struct flow_common *f, int epo= llid); > int flow_epoll_set(const struct flow_common *f, int command, uint32_t ev= ents, > 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); > int flow_migrate_source_early(struct ctx *c, const struct migrate_stage = *stage, > diff --git a/flow_table.h b/flow_table.h > index e4ff6f73c35c..3a33eef15f1e 100644 > --- a/flow_table.h > +++ b/flow_table.h > @@ -196,7 +196,7 @@ static inline flow_sidx_t flow_sidx(const struct flow= _common *f, > */ > #define FLOW_SIDX(f_, sidei) (flow_sidx(&(f_)->f, (sidei))) > =20 > -union flow *flow_alloc(void); > +union flow *flow_alloc(unsigned int qpair); > void flow_alloc_cancel(union flow *flow); > =20 > const struct flowside *flow_initiate_af(union flow *flow, uint8_t pif, > diff --git a/icmp.c b/icmp.c > index 92cb48dfaafe..e75d66d168be 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -183,11 +183,10 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > { > uint8_t proto =3D af =3D=3D AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; > uint8_t flowtype =3D af =3D=3D AF_INET ? FLOW_PING4 : FLOW_PING6; > - union flow *flow =3D flow_alloc(); > + union flow *flow =3D flow_alloc(qpair); > struct icmp_ping_flow *pingf; > const struct flowside *tgt; > =20 > - (void)qpair; > if (!flow) > return NULL; > =20 > diff --git a/tcp.c b/tcp.c > index 982f8a4a845a..39f732f8bea3 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -1707,7 +1707,7 @@ static void tcp_conn_from_tap(const struct ctx *c, = unsigned int qpair, > int s =3D -1, mss; > uint64_t hash; > =20 > - if (!(flow =3D flow_alloc())) > + if (!(flow =3D flow_alloc(qpair))) > return; > =20 > ini =3D flow_initiate_af(flow, PIF_TAP, > @@ -2304,7 +2304,7 @@ static void tcp_rst_no_conn(const struct ctx *c, un= signed int qpair, int af, > /** > * tcp_tap_handler() - Handle packets from tap and state transitions > * @c: Execution context > - * @qpair: Queue pair on which to send packets > + * @qpair: Queue pair to process This looks like a correction to an earlier patch in the series, which ideally would be folded in there. Plus, "Queue pair to process" isn't terribly informative as noted elsewhere. > * @pif: pif on which the packet is arriving > * @af: Address family, AF_INET or AF_INET6 > * @saddr: Source address > @@ -2625,7 +2625,7 @@ void tcp_listen_handler(const struct ctx *c, union = epoll_ref ref, > =20 > assert(!c->no_tcp); > =20 > - if (!(flow =3D flow_alloc())) > + if (!(flow =3D flow_alloc(QPAIR_DEFAULT))) > return; > =20 > s =3D accept4(ref.fd, &sa.sa, &sl, SOCK_NONBLOCK); > @@ -3007,6 +3007,9 @@ static void tcp_keepalive(struct ctx *c, const stru= ct timespec *now, > flow_foreach_of_type(flow, FLOW_TCP) { > struct tcp_tap_conn *conn =3D &flow->tcp; > =20 > + if (conn->f.qpair !=3D qpair) > + continue; > + > if (conn->tap_inactive) { > flow_dbg(conn, "No tap activity for least %us, send keepalive", > KEEPALIVE_INTERVAL); > @@ -3038,6 +3041,9 @@ static void tcp_inactivity(struct ctx *c, const str= uct timespec *now, > flow_foreach_of_type(flow, FLOW_TCP) { > struct tcp_tap_conn *conn =3D &flow->tcp; > =20 > + if (conn->f.qpair !=3D qpair) > + continue; > + > if (conn->inactive) { > /* No activity in this interval, reset */ > flow_dbg(conn, "Inactive for at least %us, resetting", > @@ -3828,7 +3834,7 @@ int tcp_flow_migrate_target(struct ctx *c, int fd) > union flow *flow; > int rc; > =20 > - if (!(flow =3D flow_alloc())) { > + if (!(flow =3D flow_alloc(QPAIR_DEFAULT))) { > err("Flow table full on migration target"); > return 0; > } > @@ -4050,10 +4056,10 @@ int tcp_flow_migrate_target_ext(struct ctx *c, st= ruct tcp_tap_conn *conn, > if (tcp_set_peek_offset(conn, peek_offset, now)) > goto fail; > =20 > - if (tcp_send_flag(c, conn, ACK, now, QPAIR_DEFAULT)) > + if (tcp_send_flag(c, conn, ACK, now, conn->f.qpair)) > goto fail; > =20 > - tcp_data_from_sock(c, conn, now, QPAIR_DEFAULT); > + tcp_data_from_sock(c, conn, now, conn->f.qpair); > =20 > if ((rc =3D tcp_epoll_ctl(conn))) { > flow_dbg(conn, > @@ -4071,7 +4077,7 @@ fail: > } > =20 > conn->flags =3D 0; /* Not waiting for ACK, don't schedule timer */ > - tcp_rst(c, conn, now, QPAIR_DEFAULT); > + tcp_rst(c, conn, now, conn->f.qpair); > =20 > return 0; > } > diff --git a/udp_flow.c b/udp_flow.c > index ff6abcb8eba0..2578d640a3da 100644 > --- a/udp_flow.c > +++ b/udp_flow.c > @@ -84,7 +84,6 @@ static int udp_flow_sock(const struct ctx *c, > return s; > } > =20 > - flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT); > if (flow_epoll_set(&uflow->f, EPOLL_CTL_ADD, EPOLLIN, s, sidei) < 0) { > rc =3D -errno; > close(s); > @@ -155,6 +154,7 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, = union flow *flow, > uflow->ttl[INISIDE] =3D uflow->ttl[TGTSIDE] =3D 0; > uflow->activity[INISIDE] =3D 1; > uflow->activity[TGTSIDE] =3D 0; > + flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT); Moving flow_epollid_set() looks correct (it was in a pretty odd place before). Arguably, not logically related to this patch, though. > =20 > flow_foreach_sidei(sidei) { > if (pif_is_socket(uflow->f.pif[sidei])) > @@ -236,8 +236,7 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, u= nsigned int qpair, > return flow_sidx_opposite(sidx); > } > =20 > - (void)qpair; > - if (!(flow =3D flow_alloc())) { > + if (!(flow =3D flow_alloc(qpair))) { > char sastr[SOCKADDR_STRLEN]; > =20 > err_ratelimit(now, "Couldn't allocate flow for UDP datagram from %s %s= ", > @@ -273,6 +272,7 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, u= nsigned int qpair, > * @daddr: Destination address guest side > * @srcport: Source port on guest side > * @dstport: Destination port on guest side > + * @now: Current timestamp This also looks like a (correct) fix that's more or less unrelated to the rest of this patch. > * > * Return: sidx for the destination side of the flow for this packet, or > * FLOW_SIDX_NONE if we couldn't find or create a flow. > @@ -297,8 +297,7 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c, un= signed int qpair, > return flow_sidx_opposite(sidx); > } > =20 > - (void)qpair; > - if (!(flow =3D flow_alloc())) { > + if (!(flow =3D flow_alloc(qpair))) { > char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN]; > =20 > err_ratelimit(now, "Couldn't allocate flow for UDP datagram from %s %s= :%hu -> %s:%hu", > --=20 > 2.54.0 >=20 --=20 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 --f33FcufbTC0BFc8K Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpwDZQACgkQzQJF27ox 2GcibA//d/rEkY2hkdrL3z+VW7Zc8VYfjlLUzluHIQdTTeN+iBKrsrks/dSHpDIM Jm2vKd8JX9XMpT3HC2PlHXV2TqELj/9vy6T/eMdkahuUnRF0/9NzbZQVxeExkCdo 96e+wohmMPeU3jFFDCi5+S2MkOtKXplwpDKMF6SdEzlR4LunA+83J8FJ+G/C2Io0 QZdf7RN8T9CPRgC1LuFnGGrVHlvtLVgfFIEmx6FhT+feB+y/vCWHbcFpZwiwESc8 2tvjpXb+X8H5dc+G4AyT9L/agB8UCP6ktZ2hCuJYSFnDPGhXFHPh1emaMRewdLrE S8AAM9kEXePcLKLBqcuEJ1CUWnJM/pi7nj344XaUuMH6dx8Xg+U+CR8+GiVAKEnb 3Utqhqvi05tM5OoDQshxw69J9JlsqftsYWVETw+l07f/VFckjb8o9UwSyfEBGChn GVzsuROCaN7Kk23suGUcTnm4l9Azg9t6PBSTdT22pOvdzL96UJeun4O8SQ1q3nhE Yz1yAupcbFJj/Y2fiQzZSpsw3Kve77/Y0FaGPPYIvwjlsXdHxp8N3nglmL57y8yE p3UyjcYNaQygZNYrFyIMhHxl/YFqT4KzTUycC5K4f3RFjE18mvfDZ6gwxIXAVNli 6g3BmjEz5C5fHZAgaqqjagBbk2Egw6iRuJx95QhBbjQOWA4x4DA= =1wuM -----END PGP SIGNATURE----- --f33FcufbTC0BFc8K--