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 <[email protected]>
> Cc: Yi-Hung Wei <[email protected]>
> Signed-off-by: William Tu <[email protected]>
> ---
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;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
