diff mbox series

[ovs-dev,OVS,3/4] dpif-netdev: Use the u32 instead of u64 for buckets

Message ID 1588244439-58766-4-git-send-email-xiangxia.m.yue@gmail.com
State Changes Requested
Headers show
Series expand the meter table and fix bug | expand

Commit Message

Tonghao Zhang April 30, 2020, 11 a.m. UTC 
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

When setting the meter rate to 4+Gbps, there is an overflow, the
meters don't work as expected.

Cc: Ilya Maximets <i.maximets@ovn.org>
Cc: William Tu <u9012063@gmail.com>
Cc: Jarno Rajahalme <jarno@ovn.org>
Cc: Ben Pfaff <blp@ovn.org>
Cc: Andy Zhou <azhou@ovn.org>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 include/openvswitch/ofp-meter.h | 2 +-
 lib/dpif-netdev.c               | 4 ++--
 lib/ofp-meter.c                 | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

Comments

William Tu May 8, 2020, 11:12 p.m. UTC | #1 
On Thu, Apr 30, 2020 at 07:00:38PM +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> When setting the meter rate to 4+Gbps, there is an overflow, the
> meters don't work as expected.
> 
> Cc: Ilya Maximets <i.maximets@ovn.org>
> Cc: William Tu <u9012063@gmail.com>
> Cc: Jarno Rajahalme <jarno@ovn.org>
> Cc: Ben Pfaff <blp@ovn.org>
> Cc: Andy Zhou <azhou@ovn.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  include/openvswitch/ofp-meter.h | 2 +-
>  lib/dpif-netdev.c               | 4 ++--
>  lib/ofp-meter.c                 | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> index 6776eae87e26..f55f89ac1a71 100644
> --- a/include/openvswitch/ofp-meter.h
> +++ b/include/openvswitch/ofp-meter.h
> @@ -37,7 +37,7 @@ struct ofputil_meter_band {
>      uint16_t type;
>      uint8_t prec_level;         /* Non-zero if type == OFPMBT_DSCP_REMARK. */
>      uint32_t rate;
> -    uint32_t burst_size;
> +    uint64_t burst_size;
>  };
>  
>  void ofputil_format_meter_band(struct ds *, enum ofp13_meter_flags,
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 59546db6a2a2..104347d8b251 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -277,7 +277,7 @@ static bool dpcls_lookup(struct dpcls *cls,
>  
>  struct dp_meter_band {
>      struct ofputil_meter_band up; /* type, prec_level, pad, rate, burst_size */
> -    uint32_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
> +    uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */

why setting to 4Gbps will overflow?
Each unit in bucket is 1kbpf, so 4Gbps is around 4M, around 2^20.

William

>      uint64_t packet_count;
>      uint64_t byte_count;
>  };
> @@ -5970,7 +5970,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>          band = &meter->bands[m];
>  
>          /* Update band's bucket. */
> -        band->bucket += delta_t * band->up.rate;
> +        band->bucket += (uint64_t)delta_t * band->up.rate;
>          band->bucket += delta_in_us * band->up.rate / 1000;
>          if (band->bucket > band->up.burst_size) {
>              band->bucket = band->up.burst_size;
> diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
> index 9ea40a0bfb63..1ac993bb028b 100644
> --- a/lib/ofp-meter.c
> +++ b/lib/ofp-meter.c
> @@ -72,7 +72,7 @@ ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
>      ds_put_format(s, " rate=%"PRIu32, mb->rate);
>  
>      if (flags & OFPMF13_BURST) {
> -        ds_put_format(s, " burst_size=%"PRIu32, mb->burst_size);
> +        ds_put_format(s, " burst_size=%"PRIu64, mb->burst_size);
>      }
>      if (mb->type == OFPMBT13_DSCP_REMARK) {
>          ds_put_format(s, " prec_level=%"PRIu8, mb->prec_level);
> @@ -703,7 +703,7 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
>                      return error;
>                  }
>              } else if (!strcmp(name, "burst_size")) {
> -                char *error = str_to_u32(value, &band->burst_size);
> +                char *error = str_to_u64(value, &band->burst_size);
>                  if (error) {
>                      return error;
>                  }
> -- 
> 1.8.3.1
>
Tonghao Zhang May 9, 2020, 12:46 a.m. UTC | #2 
On Sat, May 9, 2020 at 7:12 AM William Tu <u9012063@gmail.com> wrote:
>
> On Thu, Apr 30, 2020 at 07:00:38PM +0800, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > When setting the meter rate to 4+Gbps, there is an overflow, the
> > meters don't work as expected.
> >
> > Cc: Ilya Maximets <i.maximets@ovn.org>
> > Cc: William Tu <u9012063@gmail.com>
> > Cc: Jarno Rajahalme <jarno@ovn.org>
> > Cc: Ben Pfaff <blp@ovn.org>
> > Cc: Andy Zhou <azhou@ovn.org>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  include/openvswitch/ofp-meter.h | 2 +-
> >  lib/dpif-netdev.c               | 4 ++--
> >  lib/ofp-meter.c                 | 4 ++--
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> > index 6776eae87e26..f55f89ac1a71 100644
> > --- a/include/openvswitch/ofp-meter.h
> > +++ b/include/openvswitch/ofp-meter.h
> > @@ -37,7 +37,7 @@ struct ofputil_meter_band {
> >      uint16_t type;
> >      uint8_t prec_level;         /* Non-zero if type == OFPMBT_DSCP_REMARK. */
> >      uint32_t rate;
> > -    uint32_t burst_size;
> > +    uint64_t burst_size;
> >  };
> >
> >  void ofputil_format_meter_band(struct ds *, enum ofp13_meter_flags,
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 59546db6a2a2..104347d8b251 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -277,7 +277,7 @@ static bool dpcls_lookup(struct dpcls *cls,
> >
> >  struct dp_meter_band {
> >      struct ofputil_meter_band up; /* type, prec_level, pad, rate, burst_size */
> > -    uint32_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
> > +    uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
>
> why setting to 4Gbps will overflow?
> Each unit in bucket is 1kbpf, so 4Gbps is around 4M, around 2^20.
Hi William, thanks for your review,
If we set the rate to 4300000kbps == 4.3Gbps
ovs-ofctl -O OpenFlow13 add-meter br-int "meter=104 kbps stats
bands=type=drop rate=4300000"

In the dpif_netdev_meter_set function:
meter->bands[i].up.burst_size *= 1000;
burst_size should be 4300000000 but the max u32 is 4294967296

> William
>
> >      uint64_t packet_count;
> >      uint64_t byte_count;
> >  };
> > @@ -5970,7 +5970,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
> >          band = &meter->bands[m];
> >
> >          /* Update band's bucket. */
> > -        band->bucket += delta_t * band->up.rate;
> > +        band->bucket += (uint64_t)delta_t * band->up.rate;
> >          band->bucket += delta_in_us * band->up.rate / 1000;
> >          if (band->bucket > band->up.burst_size) {
> >              band->bucket = band->up.burst_size;
> > diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
> > index 9ea40a0bfb63..1ac993bb028b 100644
> > --- a/lib/ofp-meter.c
> > +++ b/lib/ofp-meter.c
> > @@ -72,7 +72,7 @@ ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
> >      ds_put_format(s, " rate=%"PRIu32, mb->rate);
> >
> >      if (flags & OFPMF13_BURST) {
> > -        ds_put_format(s, " burst_size=%"PRIu32, mb->burst_size);
> > +        ds_put_format(s, " burst_size=%"PRIu64, mb->burst_size);
> >      }
> >      if (mb->type == OFPMBT13_DSCP_REMARK) {
> >          ds_put_format(s, " prec_level=%"PRIu8, mb->prec_level);
> > @@ -703,7 +703,7 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
> >                      return error;
> >                  }
> >              } else if (!strcmp(name, "burst_size")) {
> > -                char *error = str_to_u32(value, &band->burst_size);
> > +                char *error = str_to_u64(value, &band->burst_size);
> >                  if (error) {
> >                      return error;
> >                  }
> > --
> > 1.8.3.1
> >
William Tu May 9, 2020, 3:20 p.m. UTC | #3 
On Sat, May 09, 2020 at 08:46:56AM +0800, Tonghao Zhang wrote:
> On Sat, May 9, 2020 at 7:12 AM William Tu <u9012063@gmail.com> wrote:
> >
> > On Thu, Apr 30, 2020 at 07:00:38PM +0800, xiangxia.m.yue@gmail.com wrote:
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > When setting the meter rate to 4+Gbps, there is an overflow, the
> > > meters don't work as expected.
> > >
> > > Cc: Ilya Maximets <i.maximets@ovn.org>
> > > Cc: William Tu <u9012063@gmail.com>
> > > Cc: Jarno Rajahalme <jarno@ovn.org>
> > > Cc: Ben Pfaff <blp@ovn.org>
> > > Cc: Andy Zhou <azhou@ovn.org>
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > ---
> > >  include/openvswitch/ofp-meter.h | 2 +-
> > >  lib/dpif-netdev.c               | 4 ++--
> > >  lib/ofp-meter.c                 | 4 ++--
> > >  3 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
> > > index 6776eae87e26..f55f89ac1a71 100644
> > > --- a/include/openvswitch/ofp-meter.h
> > > +++ b/include/openvswitch/ofp-meter.h
> > > @@ -37,7 +37,7 @@ struct ofputil_meter_band {
> > >      uint16_t type;
> > >      uint8_t prec_level;         /* Non-zero if type == OFPMBT_DSCP_REMARK. */
> > >      uint32_t rate;
> > > -    uint32_t burst_size;
> > > +    uint64_t burst_size;
> > >  };
> > >
> > >  void ofputil_format_meter_band(struct ds *, enum ofp13_meter_flags,
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index 59546db6a2a2..104347d8b251 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -277,7 +277,7 @@ static bool dpcls_lookup(struct dpcls *cls,
> > >
> > >  struct dp_meter_band {
> > >      struct ofputil_meter_band up; /* type, prec_level, pad, rate, burst_size */
> > > -    uint32_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
> > > +    uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
> >
> > why setting to 4Gbps will overflow?
> > Each unit in bucket is 1kbpf, so 4Gbps is around 4M, around 2^20.
> Hi William, thanks for your review,
> If we set the rate to 4300000kbps == 4.3Gbps
> ovs-ofctl -O OpenFlow13 add-meter br-int "meter=104 kbps stats
> bands=type=drop rate=4300000"
> 
> In the dpif_netdev_meter_set function:
> meter->bands[i].up.burst_size *= 1000;
> burst_size should be 4300000000 but the max u32 is 4294967296
> 
Now I understand, thanks.

Acked-by: William Tu <u9012063@gmail.com>
diff mbox series

Patch

diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
index 6776eae87e26..f55f89ac1a71 100644
--- a/include/openvswitch/ofp-meter.h
+++ b/include/openvswitch/ofp-meter.h
@@ -37,7 +37,7 @@  struct ofputil_meter_band {
     uint16_t type;
     uint8_t prec_level;         /* Non-zero if type == OFPMBT_DSCP_REMARK. */
     uint32_t rate;
-    uint32_t burst_size;
+    uint64_t burst_size;
 };
 
 void ofputil_format_meter_band(struct ds *, enum ofp13_meter_flags,
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 59546db6a2a2..104347d8b251 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -277,7 +277,7 @@  static bool dpcls_lookup(struct dpcls *cls,
 
 struct dp_meter_band {
     struct ofputil_meter_band up; /* type, prec_level, pad, rate, burst_size */
-    uint32_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
+    uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
     uint64_t packet_count;
     uint64_t byte_count;
 };
@@ -5970,7 +5970,7 @@  dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
         band = &meter->bands[m];
 
         /* Update band's bucket. */
-        band->bucket += delta_t * band->up.rate;
+        band->bucket += (uint64_t)delta_t * band->up.rate;
         band->bucket += delta_in_us * band->up.rate / 1000;
         if (band->bucket > band->up.burst_size) {
             band->bucket = band->up.burst_size;
diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
index 9ea40a0bfb63..1ac993bb028b 100644
--- a/lib/ofp-meter.c
+++ b/lib/ofp-meter.c
@@ -72,7 +72,7 @@  ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
     ds_put_format(s, " rate=%"PRIu32, mb->rate);
 
     if (flags & OFPMF13_BURST) {
-        ds_put_format(s, " burst_size=%"PRIu32, mb->burst_size);
+        ds_put_format(s, " burst_size=%"PRIu64, mb->burst_size);
     }
     if (mb->type == OFPMBT13_DSCP_REMARK) {
         ds_put_format(s, " prec_level=%"PRIu8, mb->prec_level);
@@ -703,7 +703,7 @@  parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string,
                     return error;
                 }
             } else if (!strcmp(name, "burst_size")) {
-                char *error = str_to_u32(value, &band->burst_size);
+                char *error = str_to_u64(value, &band->burst_size);
                 if (error) {
                     return error;
                 }