On 01 Sep 2026 at 10:17:58 AM, Aaron Conole <[email protected]> wrote:
> Felix Huettner <[email protected]> writes: > >> Am Mon, Aug 24, 2026 at 12:44:53PM +0200 schrieb Paolo Valerio: >>> 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) { >> >> Hi everyone, >> >> thanks for the review. >> >>> > >>> > 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). >> >> Oh, that was not intended. I will change this in v5 so that we >> actually continue on the current zone if we exited due to the >> `conn_count > limit` condition. >> >>> > >>> > 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. >>> > >> >> I am not sure how an ordered list could be reasonably maintained. >> conn_update_expiration is called for each packet of that connection, not >> just for new connections. So this means that each single packet would >> potentially invalidate the ordering of this list. >> >> This in turn means that whatever calls conn_update_expiration can not update >> the list as well. It would most probably need to take a lock for the list. >> Since that lock would be taken for each individual packet i do not see >> how this could work without a lot of contention (unless we build a >> special datastructure just for this usecase). >> >> Alternatively some other thread regularly goes through the list and >> reorderes it. But if that separate thread needs to read all expirations >> of all connections it can as well just check and delete these expiration >> values. >> >>> >>> 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). >> >> My goal here was to isolate individual zones from each other. So that if >> one zone has a gigantic amount (or insertion rate) of connections it does not >> starve the other ones in some way. >> >> This could be improved in the future with parallel cleaning of different >> zones >> by different threads. That is why i would want to align all locks a cleaning >> thread needs to take. If there would be multiple cleaning threads each >> handling their own zone but needing to lock a common datastructure the >> cleaning would be significantly less efficient. >> >> >> For non-zone cases i honestly do not think there is much of a >> difference. With the exp_lists we would have iterated over the different >> lists until we where at 1/64 of max connections. Since connections where >> randomly distributed over the exp_lists we would basically iterate >> through all connections once in a random order. >> The big difference without exp_lists is that we also iterate over >> connections with CT_DIR_REV and need to ignore these. >> >> So while the method of doing that iteration is different we generally >> touch all connections in a random order. >> >>> >>> 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. >> >> Would you have a reference to these approaches? I would be interested to >> read up on them. > > I think it was mostly discussed about 4-ish years ago. The last I > remember about it, we shelved the effort for moving to buckets because > of how close it was to the branching point: > > https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395741.html > Yes, I recall the same, we were close to the brancing point plus the fact there was no solid eviction strategy ready yet (other than iterating all the way through the cmaps in each bucket), and a few things to dig a little deeper, like balancing (and if that could become a problem). As a side note, apparently, I'm having hard time with patchwork, but the thread should reference the last iteration (v4). > Basically, for the scenarios we were mostly seeing / interested in, the > timeout policy changes gave 'good enough' performance for the scenarios > we cared about. > > But also, it does raise an important carryover part of that discussion - > we don't really have any good benchmarking for ct userspace apart from > these synthetic benchmarks (ie: in-tree 'unit test' like benchmarks > rather than a live running benchmark). IMO, it may be a good parallel > effort to get something together (although I don't quite have the cycles > to work on it at the moment). > > Each approach will give some scenario of traffic handling more > performance at the cost of some other scenario, and the important part > is to make sure we have the most wide net possible for the different > versions, and it would be good if we had a way to generate a diverse set > of conntrack tests to capture them for comparison. > I agree, having broarder benchmarking strategies certainly helps. Even simple features may introduce a penalty in a specific scenario (e.g. with additional lookups, unwanted contention, and so forth). >> Thanks, >> Felix >> >>> >>> > >>> >> + 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
