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, mit...@outlook.com wrote:
From: Lin Huang <linhu...@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 <linhu...@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
--
Best regards, Huang Lin.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
