Message ID | 20210121094210.55038-1-xiangxia.m.yue@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,ovs,v2,1/3] dpif-netdev: Fix the meter buckets overflow. | expand |
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 >
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; }