[ovs-dev] [PATCH RESEND v2 1/3] dpif-netdev: Fix the meter buckets overflow.
Ilya Maximets
i.maximets at ovn.org
Thu Feb 25 20:23:53 UTC 2021
On 2/24/21 1:21 PM, xiangxia.m.yue at gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue at 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 at gmail.com>
> ---
Hi. Thanks for working on this and sorry again for delays.
Looking at this patch I see that it actually does much more than
just fixing the overflow, so it should be split at least in 2 separate
patches:
1. Just fix the overflow by removing the 'up' and having 64bit fields
in a local structure.
2. Re-working of the way how kilobits translated to bits and other stuff
including max_delta_t and band->bucket calculations.
2.a There is also a change that we're starting form the empty bucket vs
starting with a full bucket. Not sure if this should be a separate
patch because it likely affects the refactoring part in point #2.
But anyway, this is a change of a behavior that needs to be mentioned
in a commit message at least.
For the series:
Tests should pass regardless of how many patches of the series applied,
so tests should be fixed along with the code changes in the same patch.
Some comments inline.
Best regards, Ilya Maximets.
> 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 4381c618f1be..614f6fef6b77 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;
> };
> @@ -6203,12 +6205,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;
Please, add a space between cast and a variable. I know that other
code in this function is not written this way, but it's not a reason
to follow a bad pattern.
> + if (band->bucket > max_bucket_size) {
> + band->bucket = max_bucket_size;
> }
>
> /* Drain the bucket for all the packets, if possible. */
> @@ -6226,8 +6230,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;
> }
> }
> @@ -6246,8 +6250,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;
> }
> }
> @@ -6324,21 +6328,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;
> - }
This condition is removed, but it's important and should be kept.
It's not fully correct though, we should check for
(config->flags & OFPMF13_BURST) instead and set 'burst_size' to 'rate'
if flag is not set. I'm not 100% sure about choosing the 'rate' as
its own granularity value, but that is what kernel datapath does,
so userspace datapath should behave in a same way.
> -
> - 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;
> }
>
More information about the dev
mailing list