[ovs-dev] dpif-netdev: fix meter granulariy.
diff mbox series

Message ID 1554832745-26792-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev] dpif-netdev: fix meter granulariy.
Related show

Commit Message

William Tu April 9, 2019, 5:59 p.m. UTC
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@samsung.com>
Cc: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: William Tu <u9012063@gmail.com>
---
 lib/dpif-netdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Ilya Maximets April 10, 2019, 12:14 p.m. UTC | #1
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@samsung.com>
> Cc: Yi-Hung Wei <yihung.wei@gmail.com>
> Signed-off-by: William Tu <u9012063@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 */

I'm not much familiar with meters here. Is it OK if 'band->bucket'
grows only if we're crossing millisecond boundary?
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;
>
William Tu April 11, 2019, 9:21 p.m. UTC | #2
On Wed, Apr 10, 2019 at 5:14 AM Ilya Maximets <i.maximets@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@samsung.com>
> > Cc: Yi-Hung Wei <yihung.wei@gmail.com>
> > Signed-off-by: William Tu <u9012063@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)

>
> 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;
> >
Ilya Maximets April 12, 2019, 8:08 a.m. UTC | #3
On 12.04.2019 0:21, William Tu wrote:
> On Wed, Apr 10, 2019 at 5:14 AM Ilya Maximets <i.maximets@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@samsung.com>
>>> Cc: Yi-Hung Wei <yihung.wei@gmail.com>
>>> Signed-off-by: William Tu <u9012063@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;
>>>
> 
>
William Tu April 12, 2019, 9:57 p.m. UTC | #4
On Fri, Apr 12, 2019 at 1:08 AM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> On 12.04.2019 0:21, William Tu wrote:
> > On Wed, Apr 10, 2019 at 5:14 AM Ilya Maximets <i.maximets@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@samsung.com>
> >>> Cc: Yi-Hung Wei <yihung.wei@gmail.com>
> >>> Signed-off-by: William Tu <u9012063@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
>

Hi Ilya,

Oh I see your point.
So if we only update 'meter->used' when cross millisecond boundary,
then this will work.
Let me test it.

Thanks
William

> >
> >>
> >> 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;
> >>>
> >
> >

Patch
diff mbox series

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;