| Message ID | 1554832745-26792-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: fix meter granulariy. | expand |
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; >
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; > >
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; >>> > >
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; > >>> > > > >
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;
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(-)