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, [email protected] wrote: >> >>> From: Lin Huang <[email protected]> >>> >>> 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 <[email protected]> >> >> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
