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

[William Tu]

On Fri, Apr 12, 2019 at 1:08 AM Ilya Maximets <i.maximets at 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 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
>

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