[ovs-dev] [PATCH RESEND ovs v3 4/4] dpif: Don't set "burst_size" to "rate" if not specified.

Tonghao Zhang xiangxia.m.yue at gmail.com
Thu Apr 1 11:53:25 UTC 2021


On Wed, Mar 31, 2021 at 10:26 AM Tonghao Zhang <xiangxia.m.yue at gmail.com> wrote:
>
> On Wed, Mar 31, 2021 at 6:29 AM Jean Tourrilhes <jean.tourrilhes at hpe.com> wrote:
> >
> > On Tue, Mar 30, 2021 at 02:27:11PM -0700, Ben Pfaff wrote:
> > > On Tue, Mar 30, 2021 at 11:16:48PM +0200, Ilya Maximets wrote:
> > > >
> > > > OpenFlow spec is a bit loose in definition of what should
> > > > be behavior if burst is not set:
> > > > """
> > > > If the flag OFPMF_BURST is not set the burst_size values from meter
> > > > bands are ignored, and if the meter implementation uses a burst value,
> > > > this burst value must be set to an implementation defined optimal value.
> > > > """
> > > >
> > > > In our case, historically, "implementation defined optimal value" was
> > > > value equal to rate.  I have no idea why, but it's hard to argue with
> > > > it since the spec gives a great freedom to choose.
> > > >
> > > > Actually, the "burst" itself as a term makes very little sense to me.
> > > > It's defined by the spec as:
> > > > """
> > > > It defines the granularity of the meter band, for all packet or byte
> > > > bursts whose length is greater than burst value, the meter rate will
> > > > always be strictly enforced.
> > > > """
> > > >
> > > > But what is the burst?  How the implementation should define which
> > > > packets are in the burst and which are from the next one?
> > > >
> > > > Current implementation just assumes that bursts are measured per second.
> > > > But the rate is measured per second too.  So, burst and rate is
> > > > essentially the same thing and implementations just sums them together
> > > > to get the bucket size.  So, I do not understand why "burst" and
> > > > "burst_size" exist at all.  Why not just set the rate a bit higher?
> > > >
> > > > Ben, can you shed some light on this?  What was the original idea
> > > > behind the meter burst?  Or maybe I'm missing something?
> >
> >         I don't understand how you can confuse a rate and a size. The
> > OpenFlow spec clearly says it's in kilobits or packets (not per
> > seconds).
> >         A basic token bucket has only two parameters, the commited
> > rate and the burst size (i.e. maximum number of tokens in the
> > bucket). The spec reflect that in a generic way to avoid mandating an
> > implementation.
> >         Burst rate is only defined for more fancy rate limiters, such
> > as two colors rate limiters. In this case, you also have two burst
> > size, one for each token bucket. The OpenFlow spec does not support
> > those extra parameters (as of version 1.5.1).
> >         For Linux 'police' filter : rate == rate ; burst_size == burst
> >         For Linux 'htb' qdisc : rate == rate ; burst_size == burst ;
> Yes, we do also this in the kernel datapath, but not userspace datapath.
> > ceil and cburst are not supported.
> >
> > > I wasn't really involved in the design of meters.  I saw them as a
> > > feature of hardware switches that was not very relevant to software
> > > switches.  I guess I was wrong.
> > >
> > > I think that Jean Tourillhes was the primary architect of meters in
> > > OpenFlow.  I am adding him to this thread.  I hope that he can help.
> >
> >         Have fun...
> >
> >         Jean
Hi Ben, Ilya
Try to explain this patch again. Now OvS has supported the burst_size,
 as one user case,
if users don't use the burst_size feature, we should set burst_size to
rate or 0. This patch set this to 0.

As Ilya said, we should check the OFPMF13_BURST in userspace datapath,
I think it's right.

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 251788b04965..afd698be1a59 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6349,13 +6349,12 @@ 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;
+        /* Set burst size to a workable value if specified. */
+        if (config->flags & OFPMF13_BURST) {
+            meter->bands[i].burst_size = config->bands[i].burst_size;
         }

         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].burst_size + meter->bands[i].rate) * 1000ULL;
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index ceb56c6851c6..f3db0c6802b9 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3761,7 +3761,7 @@ dpif_netlink_meter_set__(struct dpif *dpif_,
ofproto_meter_id meter_id,
         nl_msg_put_u32(&buf, OVS_BAND_ATTR_RATE, band->rate);
         nl_msg_put_u32(&buf, OVS_BAND_ATTR_BURST,
                        config->flags & OFPMF13_BURST ?
-                       band->burst_size : band->rate);
+                       band->burst_size : 0);
         nl_msg_end_nested(&buf, band_offset);
     }
     nl_msg_end_nested(&buf, bands_offset);


>
>
> --
> Best regards, Tonghao



-- 
Best regards, Tonghao


More information about the dev mailing list