diff mbox series

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

Message ID SY4PR01MB8438B623B10C0D40A64B3E2CCD2BA@SY4PR01MB8438.ausprd01.prod.outlook.com
State Changes Requested
Headers show
Series netdev-dpdk: Add support for userspace port-based packet-per-second policing. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

miter July 1, 2023, 2:40 p.m. UTC 
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>
---
 include/openvswitch/token-bucket.h | 3 ++-
 lib/token-bucket.c                 | 4 ++--
 lib/vlog.c                         | 3 ++-
 ofproto/pinsched.c                 | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

Comments

Eelco Chaudron July 14, 2023, 10:42 a.m. UTC | #1 
On 1 Jul 2023, at 16:40, 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>

Thank for the patch Lin,

However what about updating this entire patch to use time_now() instead of time_msec()? 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


> ---
>  include/openvswitch/token-bucket.h | 3 ++-
>  lib/token-bucket.c                 | 4 ++--
>  lib/vlog.c                         | 3 ++-
>  ofproto/pinsched.c                 | 2 +-
>  4 files changed, 7 insertions(+), 5 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,
> +                           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..380dcd479 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -1321,7 +1321,8 @@ 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)) {
> +    if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS,
> +                               time_msec())) {
>          time_t now = time_now();
>          if (!rl->n_dropped) {
>              rl->first_dropped = now;
> 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.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets July 14, 2023, 11:34 a.m. UTC | #2 
On 7/14/23 12:42, Eelco Chaudron wrote:
> 
> 
> On 1 Jul 2023, at 16:40, 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>
> 
> 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?

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
Eelco Chaudron July 14, 2023, 1:18 p.m. UTC | #3 
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, 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>
>>
>> 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

> 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
miter July 15, 2023, 4 a.m. UTC | #4 
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, 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>
>>> 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.
>> 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
Eelco Chaudron July 15, 2023, 8:51 a.m. UTC | #5 
Send from my phone

> Op 15 jul. 2023 om 06:01 heeft miter <miterv@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, 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>
>>>> 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. 

>>> 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.
>
miter July 16, 2023, 4:15 a.m. UTC | #6 
On 7/15/2023 4:51 PM, Eelco Chaudron wrote:
>
> Send from my phone
>
>> Op 15 jul. 2023 om 06:01 heeft miter <miterv@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, 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>
>>>>> 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.
>>
diff mbox series

Patch

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..380dcd479 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -1321,7 +1321,8 @@  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)) {
+    if (!token_bucket_withdraw(&rl->token_bucket, VLOG_MSG_TOKENS,
+                               time_msec())) {
         time_t now = time_now();
         if (!rl->n_dropped) {
             rl->first_dropped = now;
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