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