[ovs-dev] [PATCH v1 7/9] conntrack: Do not rate limit ct-sweep

Gaëtan Rivet grive at u256.net
Wed Feb 24 00:35:12 UTC 2021



On Tue, Feb 23, 2021, at 22:55, William Tu wrote:
> On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <grive at u256.net> wrote:
> >
> > The current rate limit is set to allow other threads to update the
> > connections when applicable. This was valid when taking the 'ct_lock'
> > was needed with a global critical section.
> >
> > Now that the size of the critical section for 'ct_lock' is reduced, it
> > is not necessary to rate limit calls to ct_sweep() anymore.
> >
> > Signed-off-by: Gaetan Rivet <grive at u256.net>
> > Reviewed-by: Eli Britstein <elibr at nvidia.com>
> > ---
> >  lib/conntrack.c | 24 +++++++-----------------
> >  1 file changed, 7 insertions(+), 17 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 71f79a790..1b21b79bd 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1602,20 +1602,12 @@ conntrack_clean(struct conntrack *ct, long long now)
> >   * there is an actual connection that expires, or because a new connection
> >   * might be created with the minimum timeout).
> >   *
> > - * The logic below has two goals:
> > - *
> > - * - We want to reduce the number of wakeups and batch connection cleanup
> > - *   when the load is not very high.  CT_CLEAN_INTERVAL ensures that if we
> > - *   are coping with the current cleanup tasks, then we wait at least
> > - *   5 seconds to do further cleanup.
> > - *
> > - * - We don't want to keep the map locked too long, as we might prevent
> > - *   traffic from flowing.  CT_CLEAN_MIN_INTERVAL ensures that if cleanup is
> > - *   behind, there is at least some 200ms blocks of time when the map will be
> > - *   left alone, so the datapath can operate unhindered.
> > + * We want to reduce the number of wakeups and batch connection cleanup
> > + * when the load is not very high.  CT_CLEAN_INTERVAL ensures that if we
> > + * are coping with the current cleanup tasks, then we wait at least
> > + * 5 seconds to do further cleanup.
> >   */
> 
> IIUC, it's either wait for next 5-second interval, or keep cleaning when behind.
> It depends on how fine grained people program the timeout value.
> If users program s.t like 2-second, probably in reality it takes longer
> to timeout.
> 
> 
> William
> 

Yes, if the timeout is less than CT_CLEAN_INTERVAL, it will be scheduled sooner, but it is still subject to potential backlog from ct_clean().

Removing the lower rate limit is a way to reduce this backlog, which should help getting closer to the programmed timeouts.

As the ct_lock is now taken for shorter spans, I thought it was appropriate to revisit these limits, but maybe others would prefer ct_clean to take less resources.

> >  #define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
> > -#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */
> >
> >  static void *
> >  clean_thread_main(void *f_)
> > @@ -1627,12 +1619,10 @@ clean_thread_main(void *f_)
> >          long long now = time_msec();
> >          next_wake = conntrack_clean(ct, now);
> >
> > -        if (next_wake < now) {
> > -            poll_immediate_wake();
> > -        } else if (next_wake < now + CT_CLEAN_MIN_INTERVAL) {
> > -            poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);
> > +        if (next_wake > now) {
> > +            poll_timer_wait_until(MIN(next_wake, now + CT_CLEAN_INTERVAL));
> >          } else {
> > -            poll_timer_wait_until(MAX(next_wake, now + CT_CLEAN_INTERVAL));
> > +            poll_immediate_wake();
> >          }
> >          latch_wait(&ct->clean_thread_exit);
> >          poll_block();
> > --
> > 2.30.0
> >
>

-- 
Gaetan


More information about the dev mailing list