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=ftFcoRxl; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 2A9A25A0265 for ; Mon, 03 Aug 2026 11:59:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1785751145; bh=ZkIxFW4xJeZbjdNL8/3Q/2CgWTpF9+maa0zZ3SAeql4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ftFcoRxlnrYIoVoGg6UlvJRm4oyi0mYTyC7XF3NsZJ+KPqkcKAuy606XNZnSw3vjW sYw6fhjzn+ploLjNnkuojbU9ZZAB8njCF+Xxg24y2iI1vp58ivBYBMateG5CUXyV7/ 0DFkOq9EuV/JHoC/ZreFTw6Sit3zdWjwN5eqrlwvXbd5nZ9P88lUvFGtaSneeyICT0 bvcr06K6UbcL3TpFY1RVFjkuYn2xQgL+df36sm7aViFEIqj7AiKWGIi65OS7uFgRdn D4fmfwJxGe7KosixkUGGoB3P2QlY1FzscxoyVbfdc3vkp9GDgEvUltbcxtI0z8KKpj hEKzbG7ZJzl4g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hDBw96ND7z4wCB; Mon, 03 Aug 2026 19:59:05 +1000 (AEST) Date: Mon, 3 Aug 2026 19:58:59 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 09/10] flow: Add locking, per-qpair filtering, and intermediate state handling Message-ID: References: <20260731162329.3552800-1-lvivier@redhat.com> <20260731162329.3552800-10-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="9iXuj+Agoi0LE15H" Content-Disposition: inline In-Reply-To: <20260731162329.3552800-10-lvivier@redhat.com> Message-ID-Hash: UURTH544AMDHHHMMGGDD627NQZKUNZQS X-Message-ID-Hash: UURTH544AMDHHHMMGGDD627NQZKUNZQS 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: --9iXuj+Agoi0LE15H Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 31, 2026 at 06:23:28PM +0200, Laurent Vivier wrote: > Protect flow table shared state (free list, hash table) with a > pthread_rwlock_t. Make flow_new_entry _Thread_local. >=20 > Filter flow_defer_handler() by qpair so each thread only processes > its own flows. >=20 > Hold the write lock during flow_defer_handler()'s second pass > (free-list rebuild), since the free list is shared with flow_alloc(). >=20 > Since the lock is released between flow_alloc() and FLOW_ACTIVATE(), > other threads can observe intermediate flow states (NEW, INI, TGT, > TYPED) during traversal. Adapt flow_foreach() to skip them silently > instead of logging an error, and change flow_defer_handler()'s > free-list rebuild to break cluster merging across them instead of > asserting. Oof, unfortunately, I'm not sure this is quite enough. The free list management was bespoke designed for our single threaded use case, and I'm not sure the current approach quite works any more with multithreading. More below. >=20 > Signed-off-by: Laurent Vivier > --- > flow.c | 100 ++++++++++++++++++++++++++++++++++++++++++++------- > flow_table.h | 2 +- > 2 files changed, 88 insertions(+), 14 deletions(-) >=20 > diff --git a/flow.c b/flow.c > index 59963ea5b1c2..8deca3e3c7f1 100644 > --- a/flow.c > +++ b/flow.c > @@ -12,6 +12,8 @@ > #include > #include > =20 > +#include > + > #include "util.h" > #include "ip.h" > #include "passt.h" > @@ -77,7 +79,10 @@ static_assert(ARRAY_SIZE(flow_epoll) =3D=3D FLOW_NUM_T= YPES, > /* Global Flow Table */ > =20 > /** > - * DOC: Theory of Operation - allocating and freeing flow entries > + * DOC: Theory of Operation > + * > + * Allocating and freeing flow entries > + * =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > * > * Flows are entries in flowtab[]. We need to routinely scan the whole t= able to > * perform deferred bookkeeping tasks on active entries, and sparse empt= y slots > @@ -125,11 +130,43 @@ static_assert(ARRAY_SIZE(flow_epoll) =3D=3D FLOW_NU= M_TYPES, > * when we encounter the start of a free cluster, we can immediately = skip > * past it, meaning that in practice we only need (number of active > * connections) + (number of free clusters) iterations. > + * > + * Flow table locking > + * =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > + * > + * The flow table has three pieces of shared global state: the free clus= ter > + * list (flow_first_free, flowtab[].free), the hash table (flow_hashtab[= ]), > + * and the in-progress allocation pointer (flow_new_entry). > + * > + * A pthread_rwlock_t (flow_lock) protects the free list and hash table: > + * > + * - flow_alloc() and flow_alloc_cancel() take the write lock to modify > + * the free list. > + * > + * - flow_hash_insert() and flow_hash_remove() take the write lock to > + * modify the hash table. > + * > + * - flowside_lookup() takes the read lock to traverse the hash table. > + * > + * flow_new_entry is _Thread_local, so each worker thread independently > + * tracks its own in-progress flow allocation. > + * > + * Between flow_alloc() and FLOW_ACTIVATE() (or flow_alloc_cancel()), the > + * flow is in an intermediate state (NEW, INI, TGT, or TYPED). Other > + * threads may observe these states during flow table traversal. > + * flow_foreach() skips them silently, and the free-list rebuild in > + * flow_defer_handler() breaks cluster merging across them. > + * > + * The write lock is held in flow_defer_handler() only for the second > + * pass (free-list rebuild), which modifies the global free list shared > + * with flow_alloc(). The first pass (deferred protocol handlers) runs > + * without the lock and filters by qpair, so each thread only processes > + * its own flows. The way free cluster consolidation is done is because it was near-free to do so, because we were already doing a regular linear scan through the entire table. Now that each defer handler is only handling a subset of the entries it's not so clear that this approach makes sense any more. At the very least with free consolidation passes from every thread, the effective cost might be significantly higher. > */ > =20 > unsigned flow_first_free; > union flow flowtab[FLOW_MAX]; > -static const union flow *flow_new_entry; /* =3D NULL */ > +static _Thread_local const union flow *flow_new_entry; /* =3D NULL */ > int qpair_to_fd[FLOW_QPAIR_SIZE]; > =20 > /* Hash table to index it */ > @@ -142,6 +179,8 @@ static flow_sidx_t flow_hashtab[FLOW_HASH_SIZE]; > static_assert(ARRAY_SIZE(flow_hashtab) >=3D 2 * FLOW_MAX, > "Safe linear probing requires hash table with more entries than the numb= er of sides in the flow table"); > =20 > +static pthread_rwlock_t flow_lock =3D PTHREAD_RWLOCK_INITIALIZER; > + > /** flowside_from_af() - Initialise flowside from addresses > * @side: flowside to initialise > * @af: Address family (AF_INET or AF_INET6) > @@ -592,12 +631,18 @@ void flow_activate(struct flow_common *f) > */ > union flow *flow_alloc(unsigned int qpair) > { > - union flow *flow =3D &flowtab[flow_first_free]; > + union flow *flow; > + > + pthread_rwlock_wrlock(&flow_lock); > + > + flow =3D &flowtab[flow_first_free]; > =20 > assert(!flow_new_entry); > =20 > - if (flow_first_free >=3D FLOW_MAX) > + if (flow_first_free >=3D FLOW_MAX) { > + pthread_rwlock_unlock(&flow_lock); > return NULL; > + } > =20 > assert(flow->f.state =3D=3D FLOW_STATE_FREE); > assert(flow->f.type =3D=3D FLOW_TYPE_NONE); > @@ -627,6 +672,8 @@ union flow *flow_alloc(unsigned int qpair) > flow_setqp(&flow->f, qpair); > flow_set_state(&flow->f, FLOW_STATE_NEW); > =20 > + pthread_rwlock_unlock(&flow_lock); > + > return flow; > } > =20 > @@ -638,6 +685,8 @@ union flow *flow_alloc(unsigned int qpair) > */ > void flow_alloc_cancel(union flow *flow) > { > + pthread_rwlock_wrlock(&flow_lock); > + > assert(flow_new_entry =3D=3D flow); > assert(flow->f.state =3D=3D FLOW_STATE_NEW || > flow->f.state =3D=3D FLOW_STATE_INI || > @@ -655,6 +704,8 @@ void flow_alloc_cancel(union flow *flow) > flow->free.next =3D flow_first_free; > flow_first_free =3D FLOW_IDX(flow); Alas, this is not safe. Restoring flow_first_free to the flow index relied on the fact that when single threaded, there could be no other entries allocated between flow_alloc() and FLOW_ACTIVATE(). Because we always allocated the first free entry, if we cancel it will become the first free entry again. But with the lock dropped between flow_alloc() and flow_alloc_cancel(), this could be restoring an out of date snapshot of flow_first_free - it might no longer be the actual first free entry any more if another thread freed something earlier in the table. Honestly, I'm not sure if flow_alloc_cancel() makes sense any more in a multi-threaded context. The rules about when it was and wasn't ok were already pretty confusing, so maybe we should go through the full flow close path even for flows cancelled very early. > flow_new_entry =3D NULL; > + > + pthread_rwlock_unlock(&flow_lock); > } > =20 > /** > @@ -739,10 +790,15 @@ static inline unsigned flow_hash_probe(const struct= ctx *c, flow_sidx_t sidx) > */ > uint64_t flow_hash_insert(const struct ctx *c, flow_sidx_t sidx) > { > - uint64_t hash =3D flow_sidx_hash(c, sidx); It might not be necessary to move this under the lock. This reads the contents of the individual flow (which IIUC is not covered by the flow_lock), but doesn't depend on any of the shared state data. > - unsigned b =3D flow_hash_probe_(hash, sidx); > + uint64_t hash; > + unsigned b; > =20 > + pthread_rwlock_wrlock(&flow_lock); > + hash =3D flow_sidx_hash(c, sidx); > + b =3D flow_hash_probe_(hash, sidx); > flow_hashtab[b] =3D sidx; > + pthread_rwlock_unlock(&flow_lock); > + > flow_dbg(flow_at_sidx(sidx), "Side %u hash table insert: bucket: %u", > sidx.sidei, b); > =20 > @@ -756,10 +812,15 @@ uint64_t flow_hash_insert(const struct ctx *c, flow= _sidx_t sidx) > */ > void flow_hash_remove(const struct ctx *c, flow_sidx_t sidx) > { > - unsigned b =3D flow_hash_probe(c, sidx), s; As with insert it might be worth using flow_sidx_hash() and flow_hash_probe_() separately here, so that the flow_sidx_hash() can move out of the critical section. > + unsigned b, s; > + > + pthread_rwlock_wrlock(&flow_lock); > + b =3D flow_hash_probe(c, sidx); > =20 > - if (!flow_sidx_valid(flow_hashtab[b])) > + if (!flow_sidx_valid(flow_hashtab[b])) { > + pthread_rwlock_unlock(&flow_lock); > return; /* Redundant remove */ > + } > =20 > flow_dbg(flow_at_sidx(sidx), "Side %u hash table remove: bucket: %u", > sidx.sidei, b); > @@ -779,6 +840,7 @@ void flow_hash_remove(const struct ctx *c, flow_sidx_= t sidx) > } > =20 > flow_hashtab[b] =3D FLOW_SIDX_NONE; > + pthread_rwlock_unlock(&flow_lock); > } > =20 > /** > @@ -793,10 +855,12 @@ void flow_hash_remove(const struct ctx *c, flow_sid= x_t sidx) > static flow_sidx_t flowside_lookup(const struct ctx *c, uint8_t proto, > uint8_t pif, const struct flowside *side) > { > - flow_sidx_t sidx; > + flow_sidx_t sidx, ret; > union flow *flow; > unsigned b; > =20 > + pthread_rwlock_rdlock(&flow_lock); > + > b =3D flow_hash(c, proto, pif, side) % FLOW_HASH_SIZE; As above, I think the hash can go outside the lock. > while ((sidx =3D flow_hashtab[b], flow =3D flow_at_sidx(sidx)) && > !(FLOW_PROTO(&flow->f) =3D=3D proto && > @@ -804,7 +868,11 @@ static flow_sidx_t flowside_lookup(const struct ctx = *c, uint8_t proto, > flowside_eq(&flow->f.side[sidx.sidei], side))) > b =3D mod_sub(b, 1, FLOW_HASH_SIZE); > =20 > - return flow_hashtab[b]; > + ret =3D flow_hashtab[b]; > + > + pthread_rwlock_unlock(&flow_lock); > + > + return ret; > } > =20 > /** > @@ -879,8 +947,8 @@ void flow_defer_handler(const struct ctx *c, const st= ruct timespec *now, > struct timespec *timer_run, unsigned int qpair) > { > struct flow_free_cluster *free_head =3D NULL; > - unsigned *last_next =3D &flow_first_free; > bool to_free[FLOW_MAX] =3D { 0 }; > + unsigned *last_next; > bool timer =3D false; > union flow *flow; > =20 > @@ -897,6 +965,9 @@ void flow_defer_handler(const struct ctx *c, const st= ruct timespec *now, > flow_foreach(flow) { > bool closed =3D false; > =20 > + if (flow->f.qpair !=3D qpair) > + continue; > + > switch (flow->f.type) { > case FLOW_TYPE_NONE: > assert(false); > @@ -928,6 +999,8 @@ void flow_defer_handler(const struct ctx *c, const st= ruct timespec *now, > } > =20 > /* Second step: actually free the flows */ > + pthread_rwlock_wrlock(&flow_lock); > + last_next =3D &flow_first_free; > flow_foreach_slot(flow) { > switch (flow->f.state) { > case FLOW_STATE_FREE: { > @@ -956,8 +1029,8 @@ void flow_defer_handler(const struct ctx *c, const s= truct timespec *now, > case FLOW_STATE_INI: > case FLOW_STATE_TGT: > case FLOW_STATE_TYPED: > - /* Incomplete flow at end of cycle */ > - assert(false); > + /* In-progress allocation on another thread */ > + free_head =3D NULL; I think these can all now be additional labels on the FLOW_STATE_ACTIVE case. At which point, this no longer really wants to be a switch, but rather just an if (state =3D=3D FLOW_STATE_FREE) else. > break; > =20 > case FLOW_STATE_ACTIVE: > @@ -989,6 +1062,7 @@ void flow_defer_handler(const struct ctx *c, const s= truct timespec *now, > } > =20 > *last_next =3D FLOW_MAX; > + pthread_rwlock_unlock(&flow_lock); > } > =20 > /** > diff --git a/flow_table.h b/flow_table.h > index 3a33eef15f1e..57944b748920 100644 > --- a/flow_table.h > +++ b/flow_table.h > @@ -72,7 +72,7 @@ extern union flow flowtab[]; > (flow) +=3D (flow)->free.n - 1; \ > /* NOLINTNEXTLINE(readability-inconsistent-ifelse-braces) */\ > else if ((flow)->f.state !=3D FLOW_STATE_ACTIVE) { \ > - flow_err((flow), "Bad flow state during traversal"); \ > + (void)0; /* Differs from bare continue */ \ > continue; \ > } else > =20 > --=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 --9iXuj+Agoi0LE15H Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpwZlMACgkQzQJF27ox 2Ge2vg//apIn09uGvICXNHeEKg8GEDt4luVu28fGJxg9LnCDn9PX36RDp/ja5EG9 vwI412/mzT7CMpqMKo1g2IjrZSE+9fe4kC7ZiS+KDyE6qxFHkVY2LTwQG8cabASu 9xwEFs68FG4g+CmgEipoOJEAjox3j4nJWEkDx5JynZ/Ps9RcV8cu9qafR3l9GiN0 TBQlEbNdBuxvVYJUD1+CZPlOdUrbqXXzxbUgvnr/8cQDq7CnE7TX1iVDCT5Yudxe ifFJM+E4WUXsgnEh+P78ljrlYuplzHd9gcDxsJIMvnY3WY3vmnrtJHyEhX32rNKZ eQhc5oLsTeJTYAzBNrK6GdsrmG9B15sEwAN8+qvWDQWDIXaEEhJme+rmOiGrJAoG VqMN9DI9hNtgCGe30epepv9H3lF7jQiniZhkHKr8Z1f1QXKxU9Je9Mq3pEwgIJq9 CFf40DM6TwFMeK7m5jldsH5D/8tz5Mm+mMd2BWU3MKcvme0fZIsEMQtGQIU7hzZL DVR+HCnRNBUjz+sfqY3vEewJSA6xSzUxaliVPzZglMoxyUpPblNPJ7D5vurdFjSR sa9xEXIrfuP7OQjVJtF+HYMfVuADjfN+0xk2lwTtYDEStovR7Yu6oVxvvU7eSwz0 uGAdlZrmjg1P6C5JfqiRniA5nO0N++uCbMIIfchi1o2pXY5WbFQ= =FZW5 -----END PGP SIGNATURE----- --9iXuj+Agoi0LE15H--