| Message ID | 3871314632b1a1b09b3d83dad1f73122ee1377ef.1613557616.git.grive@u256.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | conntrack: improve multithread scalability | expand |
On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <grive@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@u256.net> > Reviewed-by: Eli Britstein <elibr@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 > #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 >
On Tue, Feb 23, 2021, at 22:55, William Tu wrote: > On Wed, Feb 17, 2021 at 8:34 AM Gaetan Rivet <grive@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@u256.net> > > Reviewed-by: Eli Britstein <elibr@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 > > >
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. */ #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();