Am Mon, Aug 24, 2026 at 12:52:30PM +0200 schrieb Paolo Valerio: > On 21 Jul 2026 at 12:56:47 PM, Felix Huettner via dev > <[email protected]> wrote: > > > Previously we slept for a minimum of 200ms after each cleaning run, even > > if we did clean the majority of entries. This originally came from the > > requirement that we need to take the whole conntrack lock to cleanup > > connections and are therefor limited in the progress we can still make > > when inserting. > >
Hi Paolo, thanks for the review > > The previous method definitely needed refinement, that didn't happen so > far. > The idea of the wait though was introduce, among the other things, to > avoid hogging the CPU for long runs (it already significantly increased > the load in comparison with sorted expiration lists). > > That aside, the full lock mention still holds true, but contention could > have been reduced by decoupling from the mentioned lock. > > What we should verify here, given the patch makes things > opportunistically more aggressive in the attempt to reclaim more, > is the same as the one we should check for simple batching. In a > scenario with one or few zones, with high cps and an high expiration > rate, we should make sure this does not affect latency for inserting > threads when the sweeper goes through the batching (or at least doesn't > make it significantly worse). I guess there is two things to this. For the batching performance i guess we can discuss it on the previous patch? I provided you some values there. For the case here the idea what that if a large amount of connections can be assumed to be dead then we should continue cleaning and not just wait for 200ms. However i am unsure if 10% is an appropriate value here. It would also be fine for me to set this to significantly higher numbers. However we also do not have a big drawback if the ct thread wrongly continues cleaning directly. It should then not have too many connections to clean in the next iteration and thereby go back to the normal sleep between iterations. So we would burn some cpu cycles, but would not need to take a lock for that. If we actually run into this continuously then there needs to be a zone that gets a large amount of short lived connections all the time. But in this case from my perspective we need to optimize for connection throughput in that zone instead of individual connection latency. And the values of the previous patch show that we can more connections handled in less time with batching. I guess there is one scenario where we would actually see a performance degration but it seems quite artificial. Suppose you have a mostly empty conntrack table. Then there is some process causing a large amount of short lived connections. But before the total amount of opened connections reaches the conntrack limit the process stops again. In this case previously the wait and non-batching of ct_clean would have made these connections probably faster. But this only works as long as the external process stops before creating more connection than the limit is, otherwise we depend on the ct_clean speed again. > > > However with the now implemented locks per zone and the batching this is > > no longer needed. In addition before we could do partion zone cleanings > > we always cleaned a full zone. This meant that a single high load zone > > would still be cleaned quite fast. > > > > Signed-off-by: Felix Huettner <[email protected]> > > --- > > lib/conntrack.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 34f6dbc3f..fdf7f91bb 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -1526,8 +1526,8 @@ ct_sweep_buf(struct conntrack *ct, uint16_t zone, > > struct conn *conn_buf[100], > > > > static size_t > > ct_sweep_zone(struct conntrack *ct, uint16_t zone, long long now, > > - size_t *cleaned_count, size_t limit, > > - struct cmap_position **current_position) > > + size_t *cleaned_count, long long *min_expiration, > > + size_t limit, struct cmap_position **current_position) > > OVS_NO_THREAD_SAFETY_ANALYSIS > > { > > struct conn_key_node *keyn; > > @@ -1567,6 +1567,8 @@ ct_sweep_zone(struct conntrack *ct, uint16_t zone, > > long long now, > > if (now >= expiration) { > > conn_buf[n_conn_buf++] = conn; > > (*cleaned_count)++; > > + } else { > > + *min_expiration = MIN(*min_expiration, expiration); > > } > > > > if (n_conn_buf == CONN_BUF_SIZE) { > > @@ -1590,6 +1592,7 @@ static long long > > conntrack_clean(struct conntrack *ct, long long now) > > { > > long long next_wakeup = now + conntrack_get_sweep_interval(ct); > > + long long min_expiration = LLONG_MAX; > > unsigned int n_conn_limit, i; > > size_t clean_end, count = 0; > > size_t total_cleaned = 0; > > @@ -1600,12 +1603,12 @@ conntrack_clean(struct conntrack *ct, long long now) > > for (i = 0; i < ARRAY_SIZE(ct->zones); i++) { > > size_t cleaned = 0; > > > > - if (count > clean_end) { > > - next_wakeup = 0; > > + if (count >= clean_end) { > > break; > > } > > > > - count += ct_sweep_zone(ct, ct->current_clean_zone, now, &cleaned, > > + count += ct_sweep_zone(ct, ct->current_clean_zone, now, > > + &cleaned, &min_expiration, > > clean_end - count, > > &ct->current_clean_position); > > total_cleaned += cleaned; > > > > @@ -1618,6 +1621,16 @@ conntrack_clean(struct conntrack *ct, long long now) > > " entries in %lld msec", total_cleaned, count, > > time_msec() - now); > > > > + /* If we did clean more than 10% of our connection limit we assume that > > + * if we would continue we would also find a lot of connections to > > clean. > > + * In this case we want to rather continue immediately to ensure we get > > + * the connections removed in a high load situation. */ > > + if (total_cleaned >= (clean_end / 10)) { > > for n_conn_limit < 640 (very unlikely someone sets this low) doesn't > this always go through immediate wake? Yes thats true, i never though about that. I'll switch the >= for a > Thanks a lot, Felix > > > + next_wakeup = 0; > > + } else { > > + next_wakeup = MIN(next_wakeup, min_expiration); > > + } > > + > > return next_wakeup; > > } > > > > @@ -1625,7 +1638,6 @@ conntrack_clean(struct conntrack *ct, long long now) > > * > > * We must call conntrack_clean() periodically. conntrack_clean() return > > * value gives an hint on when the next cleanup must be done. */ > > -#define CT_CLEAN_MIN_INTERVAL_MS 200 > > > > static void * > > clean_thread_main(void *f_) > > @@ -1639,7 +1651,7 @@ clean_thread_main(void *f_) > > next_wake = conntrack_clean(ct, now); > > > > if (next_wake < now) { > > - poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL_MS); > > + poll_immediate_wake(); > > } else { > > poll_timer_wait_until(next_wake); > > } > > -- > > 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
