[ovs-dev,ovs,v2,1/3] dpif-netdev: Fix the meter buckets overflow.

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

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

$ ovs-ofctl -O OpenFlow13 add-meter br-int "meter=1 kbps stats bands=type=drop rate=4294968"

Before the patch, the buckets of meters was stored in its burst_size
of ofputil_meter_band. It was overflow when we set the rate to 4294968.
This patch don't change the public API and structure. This patch remove
the "up" from dp_meter_band, and introduce the type, rate to datapath's
meter bands. Then datapath don't depend upper layer.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/dpif-netdev.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

Bleep bloop.  Greetings Tonghao Zhang, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Inappropriate spacing around cast
#52 FILE: lib/dpif-netdev.c:6209:
        band->bucket += (uint64_t)delta_t * band->rate;

Lines checked: 110, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Thu, Jan 21, 2021 at 5:44 PM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> When setting the meter rate to 4.3+Gbps, there is an overflow, the
> meters don't work as expected.
>
> $ ovs-ofctl -O OpenFlow13 add-meter br-int "meter=1 kbps stats bands=type=drop rate=4294968"
>
> Before the patch, the buckets of meters was stored in its burst_size
> of ofputil_meter_band. It was overflow when we set the rate to 4294968.
> This patch don't change the public API and structure. This patch remove
> the "up" from dp_meter_band, and introduce the type, rate to datapath's
> meter bands. Then datapath don't depend upper layer.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
ping
> ---
>  lib/dpif-netdev.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e3fd0a07f..c281f9ac6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -279,8 +279,10 @@ static bool dpcls_lookup(struct dpcls *cls,
>      ( 1 << OFPMBT13_DROP )
>
>  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) */
> +    uint16_t type;
> +    uint32_t rate;
> +    uint32_t burst_size;
> +    uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
>      uint64_t packet_count;
>      uint64_t byte_count;
>  };
> @@ -6199,12 +6201,14 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>      /* Update all bands and find the one hit with the highest rate for each
>       * packet (if any). */
>      for (int m = 0; m < meter->n_bands; ++m) {
> -        band = &meter->bands[m];
> +        uint64_t max_bucket_size;
>
> +        band = &meter->bands[m];
>          /* Update band's bucket. */
> -        band->bucket += delta_t * band->up.rate;
> -        if (band->bucket > band->up.burst_size) {
> -            band->bucket = band->up.burst_size;
> +        max_bucket_size = band->rate * 1000ULL;
> +        band->bucket += (uint64_t)delta_t * band->rate;
> +        if (band->bucket > max_bucket_size) {
> +            band->bucket = max_bucket_size;
>          }
>
>          /* Drain the bucket for all the packets, if possible. */
> @@ -6222,8 +6226,8 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>                   * (Only one band will be fired by a packet, and that
>                   * can be different for each packet.) */
>                  for (int i = band_exceeded_pkt; i < cnt; i++) {
> -                    if (band->up.rate > exceeded_rate[i]) {
> -                        exceeded_rate[i] = band->up.rate;
> +                    if (band->rate > exceeded_rate[i]) {
> +                        exceeded_rate[i] = band->rate;
>                          exceeded_band[i] = m;
>                      }
>                  }
> @@ -6242,8 +6246,8 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
>                          /* Update the exceeding band for the exceeding packet.
>                           * (Only one band will be fired by a packet, and that
>                           * can be different for each packet.) */
> -                        if (band->up.rate > exceeded_rate[i]) {
> -                            exceeded_rate[i] = band->up.rate;
> +                        if (band->rate > exceeded_rate[i]) {
> +                            exceeded_rate[i] = band->rate;
>                              exceeded_band[i] = m;
>                          }
>                      }
> @@ -6320,21 +6324,15 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
>      for (i = 0; i < config->n_bands; ++i) {
>          uint32_t band_max_delta_t;
>
> -        /* Set burst size to a workable value if none specified. */
> -        if (config->bands[i].burst_size == 0) {
> -            config->bands[i].burst_size = config->bands[i].rate;
> -        }
> -
> -        meter->bands[i].up = config->bands[i];
> -        /* Convert burst size to the bucket units: */
> -        /* pkts => 1/1000 packets, kilobits => bits. */
> -        meter->bands[i].up.burst_size *= 1000;
> -        /* Initialize bucket to empty. */
> -        meter->bands[i].bucket = 0;
> +        meter->bands[i].type = config->bands[i].type;
> +        meter->bands[i].rate = config->bands[i].rate;
> +        meter->bands[i].burst_size = config->bands[i].burst_size;
> +        /* Start with a full bucket. */
> +        meter->bands[i].bucket = meter->bands[i].rate * 1000ULL;
>
>          /* Figure out max delta_t that is enough to fill any bucket. */
>          band_max_delta_t
> -            = meter->bands[i].up.burst_size / meter->bands[i].up.rate;
> +            = meter->bands[i].bucket / meter->bands[i].rate;
>          if (band_max_delta_t > meter->max_delta_t) {
>              meter->max_delta_t = band_max_delta_t;
>          }
> --
> 2.14.1
>