Re: [ovs-dev] [PATCH v6 1/4] token-bucket: Make token-bucket timestamp updated by caller.

[miter]

On 7/15/2023 4:51 PM, Eelco Chaudron wrote:

Send from my phone

Op 15 jul. 2023 om 06:01 heeft miter <mit...@outlook.com> het volgende 
geschreven:

Hi Eelco,

On 7/14/2023 9:18 PM, Eelco Chaudron wrote:

On 14 Jul 2023, at 13:34, Ilya Maximets wrote:

On 7/14/23 12:42, Eelco Chaudron wrote:

On 1 Jul 2023, at 16:40, 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>

Thank for the patch Lin,

However what about updating this entire patch to use time_now() instead of 
time_msec()?

Hmm.  But time_now() is in seconds.  It's a much lower resolution
for a bucket.  We'll also need to change all the users to adjust
the configuration.

Or am I missing something?

ACK my bad, my browser tab compare did not work out great :(

So my only comment would be maybe to avoid the time_now() in vlog_should_drop() 
under the mutex.

//Eelco

I think we don't need to change the time_now() in vlog_should_drop().
token_bucket_withdraw needs msec, but first_dropped needs sec.

Well you can divide by 1000 and save a syscall under the mutex.

OK.

Bets regards, Ilya Maximets.

This makes it more clear for me. I did a quick change to see how it looks.

This is what it looks like, let me know what you think:


diff --git a/include/openvswitch/token-bucket.h 
b/include/openvswitch/token-bucket.h
index d1191e956..3ae6b03fb 100644
--- a/include/openvswitch/token-bucket.h
+++ b/include/openvswitch/token-bucket.h
@@ -19,6 +19,7 @@

  #include <limits.h>
  #include <stdbool.h>
+#include <time.h>

  #ifdef __cplusplus
  extern "C" {
@@ -41,7 +42,7 @@ void token_bucket_init(struct token_bucket *,
  void token_bucket_set(struct token_bucket *,
                         unsigned int rate, unsigned int burst);
  bool token_bucket_withdraw(struct token_bucket *tb, unsigned int n,
-                           long long int now);
+                           time_t 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 60eb26e53..65fe924eb 100644
--- a/lib/token-bucket.c
+++ b/lib/token-bucket.c
@@ -60,7 +60,7 @@ token_bucket_set(struct token_bucket *tb,
   * removed) . */
  bool
  token_bucket_withdraw(struct token_bucket *tb, unsigned int n,
-                      long long int now)
+                      time_t now)
  {
      if (tb->tokens < n) {
          if (now > tb->last_fill) {
diff --git a/lib/vlog.c b/lib/vlog.c
index 380dcd479..91f2ae905 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -1320,10 +1320,10 @@ vlog_should_drop(const struct vlog_module *module, enum 
vlog_level level,
          return true;
      }

+    time_t now = time_now();
+
      ovs_mutex_lock(&rl->mutex);
-    if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS,
-                               time_msec())) {
-        time_t now = time_now();
+    if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS, now)) {
          if (!rl->n_dropped) {
              rl->first_dropped = now;
          }
diff --git a/ofproto/pinsched.c b/ofproto/pinsched.c
index a39e4d2ee..e857bf996 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, time_msec());
+    return token_bucket_withdraw(&ps->token_bucket, 1000, time_now());
  }

  void


Cheers,

Eelco

--
Best regards, Huang Lin.

--
Best regards, Huang Lin.

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev