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