| Message ID | SY4PR01MB84385EF9A26807AFFD89E7A7CDE2A@SY4PR01MB8438.ausprd01.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev-dpdk: Add support for userspace port-based packet-per-second policing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 26 Aug 2023, at 8:01, miterv@outlook.com wrote: > From: Lin Huang <linhuang@ruijie.com.cn> > > Now, token-bucket 'last_fill' is updated by token_bucket_withdraw() > itself. > Add a new function parameter 'now' to update timestamp by caller. > > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> Thanks Lin for following up, and sorry for the late review. Two small comments inline. Cheers, Eelco > --- > include/openvswitch/token-bucket.h | 3 ++- > lib/token-bucket.c | 4 ++-- > lib/vlog.c | 17 ++++++++++------- > ofproto/pinsched.c | 2 +- > 4 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/include/openvswitch/token-bucket.h > b/include/openvswitch/token-bucket.h > index 580747f61..d1191e956 100644 > --- a/include/openvswitch/token-bucket.h > +++ b/include/openvswitch/token-bucket.h > @@ -40,7 +40,8 @@ void token_bucket_init(struct token_bucket *, > unsigned int rate, unsigned int burst); > void token_bucket_set(struct token_bucket *, > unsigned int rate, unsigned int burst); > -bool token_bucket_withdraw(struct token_bucket *, unsigned int n); > +bool token_bucket_withdraw(struct token_bucket *tb, unsigned int n, No need to add *tb, just leave it as *. > + long long int now); > void token_bucket_wait_at(struct token_bucket *, unsigned int n, > const char *where); > #define token_bucket_wait(bucket, n) \ > diff --git a/lib/token-bucket.c b/lib/token-bucket.c > index 0badeb46b..60eb26e53 100644 > --- a/lib/token-bucket.c > +++ b/lib/token-bucket.c > @@ -59,10 +59,10 @@ token_bucket_set(struct token_bucket *tb, > * if 'tb' contained fewer than 'n' tokens (and thus 'n' tokens could > not be > * removed) . */ > bool > -token_bucket_withdraw(struct token_bucket *tb, unsigned int n) > +token_bucket_withdraw(struct token_bucket *tb, unsigned int n, > + long long int now) > { > if (tb->tokens < n) { > - long long int now = time_msec(); > if (now > tb->last_fill) { > unsigned long long int elapsed_ull > = (unsigned long long int) now - tb->last_fill; > diff --git a/lib/vlog.c b/lib/vlog.c > index b2653142f..7a46f6eb7 100644 > --- a/lib/vlog.c > +++ b/lib/vlog.c > @@ -1312,6 +1312,9 @@ bool > vlog_should_drop(const struct vlog_module *module, enum vlog_level > level, > struct vlog_rate_limit *rl) > { > + long long int now; > + time_t now_sec; > + > if (!module->honor_rate_limits) { > return false; > } > @@ -1321,12 +1324,13 @@ vlog_should_drop(const struct vlog_module > *module, enum vlog_level level, > } > > ovs_mutex_lock(&rl->mutex); > - if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS)) { > - time_t now = time_now(); > + now = time_msec(); > + now_sec = now / 1000; Can we move this above the mutex lock, especially the time_now(), which might result in a syscall? > + if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS, > now)) { > if (!rl->n_dropped) { > - rl->first_dropped = now; > + rl->first_dropped = now_sec; > } > - rl->last_dropped = now; > + rl->last_dropped = now_sec; > rl->n_dropped++; > ovs_mutex_unlock(&rl->mutex); > return true; > @@ -1335,10 +1339,9 @@ vlog_should_drop(const struct vlog_module > *module, enum vlog_level level, > if (!rl->n_dropped) { > ovs_mutex_unlock(&rl->mutex); > } else { > - time_t now = time_now(); > unsigned int n_dropped = rl->n_dropped; > - unsigned int first_dropped_elapsed = now - rl->first_dropped; > - unsigned int last_dropped_elapsed = now - rl->last_dropped; > + unsigned int first_dropped_elapsed = now_sec - > rl->first_dropped; > + unsigned int last_dropped_elapsed = now_sec - > rl->last_dropped; > rl->n_dropped = 0; > ovs_mutex_unlock(&rl->mutex); > > diff --git a/ofproto/pinsched.c b/ofproto/pinsched.c > index 59561f076..a39e4d2ee 100644 > --- a/ofproto/pinsched.c > +++ b/ofproto/pinsched.c > @@ -184,7 +184,7 @@ get_tx_packet(struct pinsched *ps) > static bool > get_token(struct pinsched *ps) > { > - return token_bucket_withdraw(&ps->token_bucket, 1000); > + return token_bucket_withdraw(&ps->token_bucket, 1000, > time_msec()); > } > > void > -- > 2.39.3
Hi Eelco, Sorry for the late reply. A comments below. On 9/19/2023 10:57 PM, Eelco Chaudron wrote: > On 26 Aug 2023, at 8:01, miterv@outlook.com wrote: > >> From: Lin Huang <linhuang@ruijie.com.cn> >> >> Now, token-bucket 'last_fill' is updated by token_bucket_withdraw() >> itself. >> Add a new function parameter 'now' to update timestamp by caller. >> >> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > > Thanks Lin for following up, and sorry for the late review. > > Two small comments inline. > > Cheers, > > Eelco > >> --- >> include/openvswitch/token-bucket.h | 3 ++- >> lib/token-bucket.c | 4 ++-- >> lib/vlog.c | 17 ++++++++++------- >> ofproto/pinsched.c | 2 +- >> 4 files changed, 15 insertions(+), 11 deletions(-) >> >> diff --git a/include/openvswitch/token-bucket.h >> b/include/openvswitch/token-bucket.h >> index 580747f61..d1191e956 100644 >> --- a/include/openvswitch/token-bucket.h >> +++ b/include/openvswitch/token-bucket.h >> @@ -40,7 +40,8 @@ void token_bucket_init(struct token_bucket *, >> unsigned int rate, unsigned int burst); >> void token_bucket_set(struct token_bucket *, >> unsigned int rate, unsigned int burst); >> -bool token_bucket_withdraw(struct token_bucket *, unsigned int n); >> +bool token_bucket_withdraw(struct token_bucket *tb, unsigned int n, > > No need to add *tb, just leave it as *. > >> + long long int now); >> void token_bucket_wait_at(struct token_bucket *, unsigned int n, >> const char *where); >> #define token_bucket_wait(bucket, n) \ >> diff --git a/lib/token-bucket.c b/lib/token-bucket.c >> index 0badeb46b..60eb26e53 100644 >> --- a/lib/token-bucket.c >> +++ b/lib/token-bucket.c >> @@ -59,10 +59,10 @@ token_bucket_set(struct token_bucket *tb, >> * if 'tb' contained fewer than 'n' tokens (and thus 'n' tokens >> could not be >> * removed) . */ >> bool >> -token_bucket_withdraw(struct token_bucket *tb, unsigned int n) >> +token_bucket_withdraw(struct token_bucket *tb, unsigned int n, >> + long long int now) >> { >> if (tb->tokens < n) { >> - long long int now = time_msec(); >> if (now > tb->last_fill) { >> unsigned long long int elapsed_ull >> = (unsigned long long int) now - tb->last_fill; >> diff --git a/lib/vlog.c b/lib/vlog.c >> index b2653142f..7a46f6eb7 100644 >> --- a/lib/vlog.c >> +++ b/lib/vlog.c >> @@ -1312,6 +1312,9 @@ bool >> vlog_should_drop(const struct vlog_module *module, enum vlog_level >> level, >> struct vlog_rate_limit *rl) >> { >> + long long int now; >> + time_t now_sec; >> + >> if (!module->honor_rate_limits) { >> return false; >> } >> @@ -1321,12 +1324,13 @@ vlog_should_drop(const struct vlog_module >> *module, enum vlog_level level, >> } >> >> ovs_mutex_lock(&rl->mutex); >> - if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS)) { >> - time_t now = time_now(); >> + now = time_msec(); >> + now_sec = now / 1000; > > Can we move this above the mutex lock, especially the time_now(), > which might result in a syscall? > I think we can't move the time_now() above the mutex lock. If a thread need to wait for the mutex, the value of now is inaccurate. >> + if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS, >> now)) { >> if (!rl->n_dropped) { >> - rl->first_dropped = now; >> + rl->first_dropped = now_sec; >> } >> - rl->last_dropped = now; >> + rl->last_dropped = now_sec; >> rl->n_dropped++; >> ovs_mutex_unlock(&rl->mutex); >> return true; >> @@ -1335,10 +1339,9 @@ vlog_should_drop(const struct vlog_module >> *module, enum vlog_level level, >> if (!rl->n_dropped) { >> ovs_mutex_unlock(&rl->mutex); >> } else { >> - time_t now = time_now(); >> unsigned int n_dropped = rl->n_dropped; >> - unsigned int first_dropped_elapsed = now - rl->first_dropped; >> - unsigned int last_dropped_elapsed = now - rl->last_dropped; >> + unsigned int first_dropped_elapsed = now_sec - >> rl->first_dropped; >> + unsigned int last_dropped_elapsed = now_sec - rl->last_dropped; >> rl->n_dropped = 0; >> ovs_mutex_unlock(&rl->mutex); >> >> diff --git a/ofproto/pinsched.c b/ofproto/pinsched.c >> index 59561f076..a39e4d2ee 100644 >> --- a/ofproto/pinsched.c >> +++ b/ofproto/pinsched.c >> @@ -184,7 +184,7 @@ get_tx_packet(struct pinsched *ps) >> static bool >> get_token(struct pinsched *ps) >> { >> - return token_bucket_withdraw(&ps->token_bucket, 1000); >> + return token_bucket_withdraw(&ps->token_bucket, 1000, time_msec()); >> } >> >> void >> -- >> 2.39.3 >
On 3 Dec 2023, at 9:36, miter wrote: > Hi Eelco, > > Sorry for the late reply. > > A comments below. > > > On 9/19/2023 10:57 PM, Eelco Chaudron wrote: >> On 26 Aug 2023, at 8:01, miterv@outlook.com wrote: >> >>> From: Lin Huang <linhuang@ruijie.com.cn> >>> >>> Now, token-bucket 'last_fill' is updated by token_bucket_withdraw() itself. >>> Add a new function parameter 'now' to update timestamp by caller. >>> >>> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> >> >> Thanks Lin for following up, and sorry for the late review. >> >> Two small comments inline. >> >> Cheers, >> >> Eelco >> >>> --- >>> include/openvswitch/token-bucket.h | 3 ++- >>> lib/token-bucket.c | 4 ++-- >>> lib/vlog.c | 17 ++++++++++------- >>> ofproto/pinsched.c | 2 +- >>> 4 files changed, 15 insertions(+), 11 deletions(-) >>> >>> diff --git a/include/openvswitch/token-bucket.h b/include/openvswitch/token-bucket.h >>> index 580747f61..d1191e956 100644 >>> --- a/include/openvswitch/token-bucket.h >>> +++ b/include/openvswitch/token-bucket.h >>> @@ -40,7 +40,8 @@ void token_bucket_init(struct token_bucket *, >>> unsigned int rate, unsigned int burst); >>> void token_bucket_set(struct token_bucket *, >>> unsigned int rate, unsigned int burst); >>> -bool token_bucket_withdraw(struct token_bucket *, unsigned int n); >>> +bool token_bucket_withdraw(struct token_bucket *tb, unsigned int n, >> >> No need to add *tb, just leave it as *. >> >>> + long long int now); >>> void token_bucket_wait_at(struct token_bucket *, unsigned int n, >>> const char *where); >>> #define token_bucket_wait(bucket, n) \ >>> diff --git a/lib/token-bucket.c b/lib/token-bucket.c >>> index 0badeb46b..60eb26e53 100644 >>> --- a/lib/token-bucket.c >>> +++ b/lib/token-bucket.c >>> @@ -59,10 +59,10 @@ token_bucket_set(struct token_bucket *tb, >>> * if 'tb' contained fewer than 'n' tokens (and thus 'n' tokens could not be >>> * removed) . */ >>> bool >>> -token_bucket_withdraw(struct token_bucket *tb, unsigned int n) >>> +token_bucket_withdraw(struct token_bucket *tb, unsigned int n, >>> + long long int now) >>> { >>> if (tb->tokens < n) { >>> - long long int now = time_msec(); >>> if (now > tb->last_fill) { >>> unsigned long long int elapsed_ull >>> = (unsigned long long int) now - tb->last_fill; >>> diff --git a/lib/vlog.c b/lib/vlog.c >>> index b2653142f..7a46f6eb7 100644 >>> --- a/lib/vlog.c >>> +++ b/lib/vlog.c >>> @@ -1312,6 +1312,9 @@ bool >>> vlog_should_drop(const struct vlog_module *module, enum vlog_level level, >>> struct vlog_rate_limit *rl) >>> { >>> + long long int now; >>> + time_t now_sec; >>> + >>> if (!module->honor_rate_limits) { >>> return false; >>> } >>> @@ -1321,12 +1324,13 @@ vlog_should_drop(const struct vlog_module *module, enum vlog_level level, >>> } >>> >>> ovs_mutex_lock(&rl->mutex); >>> - if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS)) { >>> - time_t now = time_now(); >>> + now = time_msec(); >>> + now_sec = now / 1000; >> >> Can we move this above the mutex lock, especially the time_now(), which might result in a syscall? >> > I think we can't move the time_now() above the mutex lock. If a thread need to wait for the mutex, the value of now is inaccurate. You are right it might influence the time. >>> + if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS, now)) { >>> if (!rl->n_dropped) { >>> - rl->first_dropped = now; >>> + rl->first_dropped = now_sec; >>> } >>> - rl->last_dropped = now; >>> + rl->last_dropped = now_sec; >>> rl->n_dropped++; >>> ovs_mutex_unlock(&rl->mutex); >>> return true; >>> @@ -1335,10 +1339,9 @@ vlog_should_drop(const struct vlog_module *module, enum vlog_level level, >>> if (!rl->n_dropped) { >>> ovs_mutex_unlock(&rl->mutex); >>> } else { >>> - time_t now = time_now(); >>> unsigned int n_dropped = rl->n_dropped; >>> - unsigned int first_dropped_elapsed = now - rl->first_dropped; >>> - unsigned int last_dropped_elapsed = now - rl->last_dropped; >>> + unsigned int first_dropped_elapsed = now_sec - rl->first_dropped; >>> + unsigned int last_dropped_elapsed = now_sec - rl->last_dropped; >>> rl->n_dropped = 0; >>> ovs_mutex_unlock(&rl->mutex); >>> >>> diff --git a/ofproto/pinsched.c b/ofproto/pinsched.c >>> index 59561f076..a39e4d2ee 100644 >>> --- a/ofproto/pinsched.c >>> +++ b/ofproto/pinsched.c >>> @@ -184,7 +184,7 @@ get_tx_packet(struct pinsched *ps) >>> static bool >>> get_token(struct pinsched *ps) >>> { >>> - return token_bucket_withdraw(&ps->token_bucket, 1000); >>> + return token_bucket_withdraw(&ps->token_bucket, 1000, time_msec()); >>> } >>> >>> void >>> -- >>> 2.39.3 >> > -- > Best regards, Huang Lin.
diff --git a/include/openvswitch/token-bucket.h b/include/openvswitch/token-bucket.h index 580747f61..d1191e956 100644 --- a/include/openvswitch/token-bucket.h +++ b/include/openvswitch/token-bucket.h @@ -40,7 +40,8 @@ void token_bucket_init(struct token_bucket *, unsigned int rate, unsigned int burst); void token_bucket_set(struct token_bucket *, unsigned int rate, unsigned int burst); -bool token_bucket_withdraw(struct token_bucket *, unsigned int n); +bool token_bucket_withdraw(struct token_bucket *tb, unsigned int n, + long long int now); void token_bucket_wait_at(struct token_bucket *, unsigned int n, const char *where); #define token_bucket_wait(bucket, n) \ diff --git a/lib/token-bucket.c b/lib/token-bucket.c index 0badeb46b..60eb26e53 100644 --- a/lib/token-bucket.c +++ b/lib/token-bucket.c @@ -59,10 +59,10 @@ token_bucket_set(struct token_bucket *tb, * if 'tb' contained fewer than 'n' tokens (and thus 'n' tokens could not be * removed) . */ bool -token_bucket_withdraw(struct token_bucket *tb, unsigned int n) +token_bucket_withdraw(struct token_bucket *tb, unsigned int n, + long long int now) { if (tb->tokens < n) { - long long int now = time_msec(); if (now > tb->last_fill) { unsigned long long int elapsed_ull = (unsigned long long int) now - tb->last_fill; diff --git a/lib/vlog.c b/lib/vlog.c index b2653142f..7a46f6eb7 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -1312,6 +1312,9 @@ bool vlog_should_drop(const struct vlog_module *module, enum vlog_level level, struct vlog_rate_limit *rl) { + long long int now; + time_t now_sec; + if (!module->honor_rate_limits) { return false; } @@ -1321,12 +1324,13 @@ vlog_should_drop(const struct vlog_module *module, enum vlog_level level, } ovs_mutex_lock(&rl->mutex); - if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS)) { - time_t now = time_now(); + now = time_msec(); + now_sec = now / 1000; + if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS, now)) { if (!rl->n_dropped) { - rl->first_dropped = now; + rl->first_dropped = now_sec; } - rl->last_dropped = now; + rl->last_dropped = now_sec; rl->n_dropped++; ovs_mutex_unlock(&rl->mutex); return true; @@ -1335,10 +1339,9 @@ vlog_should_drop(const struct vlog_module *module, enum vlog_level level, if (!rl->n_dropped) { ovs_mutex_unlock(&rl->mutex); } else { - time_t now = time_now(); unsigned int n_dropped = rl->n_dropped; - unsigned int first_dropped_elapsed = now - rl->first_dropped; - unsigned int last_dropped_elapsed = now - rl->last_dropped; + unsigned int first_dropped_elapsed = now_sec - rl->first_dropped; + unsigned int last_dropped_elapsed = now_sec - rl->last_dropped; rl->n_dropped = 0; ovs_mutex_unlock(&rl->mutex); diff --git a/ofproto/pinsched.c b/ofproto/pinsched.c index 59561f076..a39e4d2ee 100644 --- a/ofproto/pinsched.c +++ b/ofproto/pinsched.c @@ -184,7 +184,7 @@ get_tx_packet(struct pinsched *ps) static bool get_token(struct pinsched *ps) { - return token_bucket_withdraw(&ps->token_bucket, 1000); + return token_bucket_withdraw(&ps->token_bucket, 1000, time_msec()); } void