On Fri, Apr 12, 2019 at 1:08 AM Ilya Maximets <[email protected]> wrote: > > On 12.04.2019 0:21, William Tu wrote: > > On Wed, Apr 10, 2019 at 5:14 AM Ilya Maximets <[email protected]> > > 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 <[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 */ > > > > 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; > >>> > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
