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