[ovs-dev] [PATCH] dpif-netdev: fix meter granulariy.

[Ilya Maximets]

On 12.04.2019 0:21, William Tu wrote:
> On Wed, Apr 10, 2019 at 5:14 AM Ilya Maximets <i.maximets at samsung.com> wrote:
>>
>> On 09.04.2019 20:59, William Tu wrote:
>>> When testing packet rate around 1Mpps with meter enabled, the frequency
>>> of hitting meter action becomes much higher, around 30us each time.
>>> As a result, the meter's calculation of 'uint32_t delta_t' becomes
>>> always 0 and meter action has no effect.  This is due to the previous
>>> commit 05f9e707e194 divides the delta by 1000, in order to convert to
>>> msec granularity.  The patch fixes it by using double to hold the delta
>>> value.
>>>
>>> Fixes: 05f9e707e194 ("dpif-netdev: Use microsecond granularity.")
>>> Cc: Ilya Maximets <i.maximets at samsung.com>
>>> Cc: Yi-Hung Wei <yihung.wei at gmail.com>
>>> Signed-off-by: William Tu <u9012063 at gmail.com>
>>> ---
>>
>> Hi. Good catch.
>>
>> In fact, we may return previous behaviour by just dividing before
>> subtracting like this:
>>     long_delta_t = now / 1000 - meter->used / 1000; /* msec */
> 
> But this is the same, the long_delta_t is still 0 because my
> (now - meter->used is less than 1000)

No, it's 1 when the time crosses millisecond boundary:

    (1015 - 985) / 1000       = 30 / 1000 = 0
    1015 / 1000 - 985 / 1000  = 1 - 0     = 1

> 
>>
>> I'm not much familiar with meters here. Is it OK if 'band->bucket'
>> grows only if we're crossing millisecond boundary?
> 
> I think that will also work.
> 
> William
>> Your change makes it grow smoothly on each call which wasn't the
>> case previously.
>>
>> Jarno, maybe you have some thoughts about this?
>>
>> Best regards, Ilya Maximets.
>>
>>>  lib/dpif-netdev.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 4d6d0c372236..61d0e9f2bf69 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -5525,8 +5525,8 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>>>      struct dp_meter *meter;
>>>      struct dp_meter_band *band;
>>>      struct dp_packet *packet;
>>> -    long long int long_delta_t; /* msec */
>>> -    uint32_t delta_t; /* msec */
>>> +    double double_delta_t; /* msec */
>>> +    double delta_t; /* msec */
>>>      const size_t cnt = dp_packet_batch_size(packets_);
>>>      uint32_t bytes, volume;
>>>      int exceeded_band[NETDEV_MAX_BURST];
>>> @@ -5549,12 +5549,12 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>>>      memset(exceeded_rate, 0, cnt * sizeof *exceeded_rate);
>>>
>>>      /* All packets will hit the meter at the same time. */
>>> -    long_delta_t = (now - meter->used) / 1000; /* msec */
>>> +    double_delta_t = (double)(now - meter->used) / 1000.0; /* msec */
>>>
>>>      /* Make sure delta_t will not be too large, so that bucket will not
>>>       * wrap around below. */
>>> -    delta_t = (long_delta_t > (long long int)meter->max_delta_t)
>>> -        ? meter->max_delta_t : (uint32_t)long_delta_t;
>>> +    delta_t = (double_delta_t > (long long int)meter->max_delta_t)
>>> +        ? (double)meter->max_delta_t : double_delta_t;
>>>
>>>      /* Update meter stats. */
>>>      meter->used = now;
>>>
> 
>