[ovs-dev] [PATCH RESEND v2 1/3] dpif-netdev: Fix the meter buckets overflow.
Tonghao Zhang
xiangxia.m.yue at gmail.com
Tue Mar 2 13:29:51 UTC 2021
On Fri, Feb 26, 2021 at 4:23 AM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> 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.
Thanks for code review. I will split this to two patches.
> 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.
Yes
> 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.
Thanks, I got it.
>
> > + 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.
I don't think we should set "burst_size" to "rate", if "rate" is 100Mbps, then
there is a 200Mbps as a burst rate, but we don't use the "burst_size".
we should fix for the kernel datapath too.
The kernel module is fine, we should fix it in lib/dpif-netlink.c ?
> > -
> > - 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;
> > }
> >
>
--
Best regards, Tonghao
More information about the dev
mailing list