On 06 Aug 2026 at 10:58:27 AM, Aaron Conole <[email protected]> wrote:
> Felix Huettner <[email protected]> writes: > >> If a single zone creates a lot of churn we would have otherwise blocked >> the clean thread for quite a while. >> >> The approach with cmap_position might have the drawback that we >> potentially skip some connections that have been inserted in the >> meantime, but cmap_cursor can not be used as we have a rcu quiesce >> period in between iterations. >> >> Signed-off-by: Felix Huettner <[email protected]> >> --- >> lib/conntrack-private.h | 11 ++++++++--- >> lib/conntrack.c | 38 ++++++++++++++++++++++++++++---------- >> 2 files changed, 36 insertions(+), 13 deletions(-) > > [...] > >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index b1ac0d9ca..eb8248ed9 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -527,6 +527,10 @@ conntrack_destroy(struct conntrack *ct) >> ovs_mutex_unlock(&ct->resources_lock); >> ovs_mutex_destroy(&ct->resources_lock); >> >> + if (ct->current_clean_position) { >> + free(ct->current_clean_position); >> + } >> + >> ipf_destroy(ct->ipf); >> free(ct); >> } >> @@ -1493,18 +1497,32 @@ conntrack_get_sweep_interval(struct conntrack *ct) >> >> static size_t >> ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now, >> - size_t *cleaned_count) >> + size_t *cleaned_count, size_t limit, >> + struct cmap_position **current_position) >> OVS_NO_THREAD_SAFETY_ANALYSIS >> { >> struct conn_key_node *keyn; >> struct conntrack_zone *cz; >> unsigned int conn_count = 0; >> - unsigned int cleaned = 0; >> struct conn *conn; >> + struct cmap_node *node; >> long long expiration; >> >> cz = zone_lookup(ct, zone); >> - CMAP_FOR_EACH (keyn, cm_node, &cz->conns) { >> + if (atomic_count_get(&cz->count) == 0) { >> + return conn_count; >> + } >> + >> + if (!*current_position) { >> + *current_position = xzalloc(sizeof(**current_position)); >> + } >> + >> + while ((node = cmap_next_position(&cz->conns, *current_position))) { >> + keyn = OBJECT_CONTAINING(node, keyn, cm_node); >> + if (conn_count > limit) { > > I think I see an issue that the cursor position isn't being updated > properly when we do the early bail, so we'll always go to the next zone > (ct->current_clean_zone++ line) but the current_clean_position indexes > are all invalid (because they will be referencing a different zone's > cmap). > > Just resetting the indexes whenever we advance the clean-zone counter is > probably good enough. It may be worth considering whether each zone > should have a cmap_position from which to continue, but it will have a > large memory footprint for a specific traffic flow case, so I'm not 100% > sure it's really worth it. In the case that we reset, though, we may > have connections that *never* expire, so it probably isn't perfect that > way either. Keeping the position details can help with that though. > Not really sure what the best approach is without a redesign of the > entire cleanup mechanism to something different. > I agree. > Actually, the ordered expiration list *does* address this a bit, because > expiration is kept oldest-first, but the issue there is the cost of the > ordered list management. Maybe it wouldn't be as bad with per-zone > ordered list rather than the single global exp list. > It wouldn't be as bad the more zones are used and generally in a balanced (simplifying a bit here as it also depends on the amount and order of refresh over time) scenario. Both are not always the case. In general, the advantages of this kind of zone partitioning may suffer more the above (and some more) depending on the use-case. With buckets (just to recall an old proposal) the problem would only be the unbalance. Ordered lists (as they were) have the issue you are pointing out plus an additional one related to potential timeout policy updates (e.g. they have to handle passing from a long timeout for whatever connection state to a shorter one). > Maybe an alternative for the cmap walking approach without exp lists > could be to have the per-zone cleanup position object, but only allocate > it when we actually have connections. That could help with some of the > memory pressure, and prevent us having to do an exp_list also. > > Paolo, what are your thoughts? It would be interesting to measure the gains of the current strategy compared to the old approach, and maybe the time spent by the sweeper in a full run with a CPS test. per-zone expiration lists are not good because when no zones are used (or other possible scenarios happen) we're back to the initial point. Local cursors, OTOH, seem a bit better, but I don't really love the idea of another per-zone duplication (unless the advantage is clear and generic). All in all, some things resemble the bucket approach, and some of the problems are the same (the sweeping part was not fully resolved). One potential solution back then involved considering introducing a different rcu based data structure, better if not resizable. Another was moving to per-bucket expiration lists (that meant around 1k lists vs 64k) with an advantage beyond the number of zones used. > >> + return conn_count; >> + } >> + >> if (keyn->dir != CT_DIR_FWD) { >> continue; >> } >> @@ -1513,12 +1531,14 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone, >> long long now, >> expiration = conn_expiration(conn); >> if (now >= expiration) { >> conn_clean(ct, conn); >> - cleaned++; >> + (*cleaned_count)++; >> } >> >> conn_count++; >> } >> - *cleaned_count = cleaned; >> + >> + free(*current_position); >> + *current_position = NULL; >> return conn_count; >> } >> >> @@ -1532,7 +1552,6 @@ conntrack_clean(struct conntrack *ct, long long now) >> unsigned int n_conn_limit, i; >> size_t clean_end, count = 0; >> size_t total_cleaned = 0; >> - uint16_t current_zone = ct->next_clean_zone; >> >> atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit); >> clean_end = n_conn_limit / 64; >> @@ -1545,16 +1564,15 @@ conntrack_clean(struct conntrack *ct, long long now) >> break; >> } >> >> - count += ct_sweep_zone(ct, current_zone, now, &cleaned); >> + count += ct_sweep_zone(ct, ct->current_clean_zone, now, &cleaned, >> + clean_end - count, >> &ct->current_clean_position); >> total_cleaned += cleaned; >> >> /* This will overflow and thereby allow us to iterate through all >> * zones. */ >> - current_zone++; >> + ct->current_clean_zone++; >> } >> >> - ct->next_clean_zone = current_zone + 1; >> - >> VLOG_DBG("conntrack cleaned %"PRIuSIZE" entries out of %"PRIuSIZE >> " entries in %lld msec", total_cleaned, count, >> time_msec() - now); _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
