Am Mon, Aug 24, 2026 at 12:49:30PM +0200 schrieb Paolo Valerio:
> On 21 Jul 2026 at 12:56:45 PM, Felix Huettner via dev
> <[email protected]> wrote:
>
> > If a lot of conn entries expired in a single zone we previously took the
> > zone_lock for each conn entry we deleted. This caused a lot of churn on
> > the lock, especially if there are parallel inserts.
> >
Hi Paolo,
thanks for the comment.
>
> that's true, but if we keep having concurrent insertions, can't this hold
> some new connection for some time?
> Also, if I'm not missing something, we're widening the critical section
> including more things than before.
I agree with both of these statements.
>
> Didn't dig into this, but there might be a chance, for the contending
> side, that the adaptive lock might give up on busy waiting and go to sleep
> (which would probably make things a little worse).
>
> Can you please elaborate on how the reduced lock acquisition improved
> the numbers?
I did some testing with this change right now and i would just share
what i learned or what i could not find out, then lets see where that
takes us:
* If i test with "ovstest test-conntrack benchmark-tcp 10 10000 10 0 100"
* Before this change the conntrack runtime was 5970ms after this
change it is 4563ms.
* If i test with "ovstest test-conntrack benchmark-tcp 10 10000 1 0 100"
* Before this change the conntrack runtime was 19440ms after this
change it is 7079ms.
* In addition before this change the testcase measured it needed to
wait for 12492ms for ct_clean since the amount of connections what
at the conntrack table limit
* I did a testcase with "test-conntrack benchmark-tcp 30 10000 1 0 10"
* Measuring the amount of futex system calls between before the patch
and after the patch did not show a meaningful difference
I tried to use a stopwatch to measure the conntrack execute times in the
testcase. But it is just too many calls and the stopwatch slowed the test down
so much that it is no longer meaningful.
This means i can not tell you how much insertions might be delayed. The
average does not help us there since that is clearly lower, but maybe
p99 latency would have been interesting.
So based on the numbers we are heavily improving the case where a single
zone is getting a large amount of new connections all the time.
I hope this is what you where asking for, if not let me know.
Thanks,
Felix
>
> > We fix this by batching the deletions on 100 entries and taking a lock
> > for all of them at once.
> >
> > Signed-off-by: Felix Huettner <[email protected]>
> > ---
> > lib/conntrack.c | 66 ++++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 52 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 12109f03b..34f6dbc3f 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -436,34 +436,34 @@ zone_limit_update(struct conntrack *ct, int32_t zone,
> > uint32_t limit)
> > }
> >
> > static void
> > -conn_clean__(struct conntrack *ct, struct conn *conn)
> > +conn_clean_protected__(struct conntrack *ct, struct conntrack_zone *cz,
> > + struct conn *conn)
> > + OVS_REQUIRES(cz->zone_lock)
> > {
> > - struct conntrack_zone *cz;
> > - uint16_t fwd_zone;
> > uint32_t hash;
> >
> > + COVERAGE_INC(conntrack_remove);
> > +
> > if (conn->alg) {
> > expectation_clean(ct, &conn->key_node[CT_DIR_FWD].key);
> > }
> >
> > - fwd_zone = conn->key_node[CT_DIR_FWD].key.zone;
> > - cz = zone_lookup(ct, fwd_zone);
> > -
> > hash = conn_key_hash(&conn->key_node[CT_DIR_FWD].key, ct->hash_basis);
> > - ovs_mutex_lock(&cz->zone_lock);
> > cmap_remove(&cz->conns,
> > &conn->key_node[CT_DIR_FWD].cm_node, hash);
> > atomic_count_dec(&cz->count);
> >
> > if (conn->nat_action) {
> > - ovs_assert(fwd_zone == conn->key_node[CT_DIR_REV].key.zone);
> > + ovs_assert(conn->key_node[CT_DIR_FWD].key.zone ==
> > + conn->key_node[CT_DIR_REV].key.zone);
> > hash = conn_key_hash(&conn->key_node[CT_DIR_REV].key,
> > ct->hash_basis);
> > cmap_remove(&cz->conns,
> > &conn->key_node[CT_DIR_REV].cm_node, hash);
> > }
> >
> > - ovs_mutex_unlock(&cz->zone_lock);
> > + ovsrcu_postpone(delete_conn, conn);
> > + atomic_count_dec(&ct->n_conn);
> > }
> >
> > /* Also removes the associated nat 'conn' from the lookup
> > @@ -476,11 +476,27 @@ conn_clean(struct conntrack *ct, struct conn *conn)
> > return;
> > }
> >
> > - COVERAGE_INC(conntrack_remove);
> > - conn_clean__(ct, conn);
> > + struct conntrack_zone *cz = zone_lookup(ct,
> > + conn->key_node[CT_DIR_FWD].key.zone);
> >
> > - ovsrcu_postpone(delete_conn, conn);
> > - atomic_count_dec(&ct->n_conn);
> > + ovs_mutex_lock(&cz->zone_lock);
> > + conn_clean_protected__(ct, cz, conn);
> > + ovs_mutex_unlock(&cz->zone_lock);
> > +}
> > +
> > +/* Also removes the associated nat 'conn' from the lookup
> > + datastructures. */
> > +static void
> > +conn_clean_protected(struct conntrack *ct, struct conntrack_zone *cz,
> > + struct conn *conn)
> > + OVS_EXCLUDED(conn->lock)
> > + OVS_REQUIRES(cz->zone_lock)
> > +{
> > + if (atomic_flag_test_and_set(&conn->reclaimed)) {
> > + return;
> > + }
> > +
> > + conn_clean_protected__(ct, cz, conn);
> > }
> >
> > static void
> > @@ -1496,6 +1512,18 @@ conntrack_get_sweep_interval(struct conntrack *ct)
> > return ms;
> > }
> >
> > +static void
> > +ct_sweep_buf(struct conntrack *ct, uint16_t zone, struct conn
> > *conn_buf[100],
> > + unsigned int n_conn_buf)
> > +{
> > + struct conntrack_zone *cz = zone_lookup(ct, zone);
> > + ovs_mutex_lock(&cz->zone_lock);
> > + for (unsigned i = 0; i < n_conn_buf; i++) {
> > + conn_clean_protected(ct, cz, conn_buf[i]);
> > + }
> > + ovs_mutex_unlock(&cz->zone_lock);
> > +}
> > +
> > static size_t
> > ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now,
> > size_t *cleaned_count, size_t limit,
> > @@ -1508,6 +1536,10 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone,
> > long long now,
> > struct conn *conn;
> > long long expiration;
> >
> > +#define CONN_BUF_SIZE 100
> > + struct conn *conn_buf[CONN_BUF_SIZE];
> > + unsigned int n_conn_buf = 0;
> > +
> > cz = zone_lookup(ct, zone);
> > if (atomic_count_get(&cz->count) == 0) {
> > return conn_count;
> > @@ -1533,12 +1565,18 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone,
> > long long now,
> > conn = CONTAINER_OF(keyn, struct conn, key_node[keyn->dir]);
> > expiration = conn_expiration(conn);
> > if (now >= expiration) {
> > - conn_clean(ct, conn);
> > + conn_buf[n_conn_buf++] = conn;
> > (*cleaned_count)++;
> > }
> >
> > + if (n_conn_buf == CONN_BUF_SIZE) {
> > + ct_sweep_buf(ct, zone, conn_buf, n_conn_buf);
> > + n_conn_buf = 0;
> > + }
> > +
> > conn_count++;
> > }
> > + ct_sweep_buf(ct, zone, conn_buf, n_conn_buf);
> >
> > free(*current_position);
> > *current_position = NULL;
> > --
> > 2.43.0
> >
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
