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