[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
Wed Mar 31 02:22:50 UTC 2021


On Wed, Mar 31, 2021 at 5:16 AM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 3/3/21 3:46 PM, xiangxia.m.yue at gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> >
> > If user don't set the meter "burst_size", when creating them. OvS
> > will set "brust_size" to "rate", and there will be a double "rate"
> > as burst rate. For example:
> >
> > $ ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps stats bands=type=drop rate=1000000'
> >
> > The rate expected is 1Gbps, but burst rate is 2Gbps while
> > we don't use "burst_size".
>
>
> 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?
As I test if we set the burst_size, for example
$ ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps stats
bands=type=drop rate=1000000'
bandwidth is 2Gbps in the first second, and later 1Gbps. The
burst_size only affects the rate in the first second.

> Ben, can you shed some light on this?  What was the original idea
> behind the meter burst?  Or maybe I'm missing something?
>
> Some review comments inline.
>
> Best regards, Ilya Maximets.
>
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > ---
> >  lib/dpif-netdev.c    |   5 ---
> >  lib/dpif-netlink.c   |   2 +-
> >  tests/dpif-netdev.at | 103 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 104 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index e53ed31b906c..e0a44abebda6 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -6327,11 +6327,6 @@ 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].rate = config->bands[i].rate;
> >          meter->bands[i].burst_size = config->bands[i].burst_size;
>
> I still think that we need to check for OFPMF13_BURST in flags
> and use config->bands[i].burst_size only if it is set.
Hi I guess bust_size and rate is different, in the kernel datapath:
band->rate = nla_get_u32(attr[OVS_BAND_ATTR_RATE]);
band->burst_size = nla_get_u32(attr[OVS_BAND_ATTR_BURST]);

> >          /* Start with a full bucket. */
> > 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);
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index b2ff69a01ee6..b18297983007 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -372,6 +372,109 @@ recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> >
> > +AT_SETUP([dpif-netdev - meters without burst_size])
> > +# Create br0 with interfaces p1 and p7
> > +#    and br1 with interfaces p2 and p8
> > +# with p1 and p2 connected via unix domain socket
> > +OVS_VSWITCHD_START(
> > +  [add-port br0 p1 -- set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p0.sock ofport_request=1 -- \
> > +   add-port br0 p7 -- set interface p7 ofport_request=7 type=dummy -- \
> > +   add-br br1 -- \
> > +   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> > +   set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> > +                  fail-mode=secure -- \
> > +   add-port br1 p2 -- set interface p2 type=dummy options:stream=unix:$OVS_RUNDIR/p0.sock ofport_request=2 -- \
> > +   add-port br1 p8 -- set interface p8 ofport_request=8 type=dummy --])
> > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps stats bands=type=drop rate=1'])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=2 kbps stats bands=type=drop rate=1'])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 action=meter:1,7'])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=7 action=meter:2,1'])
> > +AT_CHECK([ovs-ofctl add-flow br1 'in_port=2 action=8'])
> > +AT_CHECK([ovs-ofctl add-flow br1 'in_port=8 action=2'])
> > +ovs-appctl time/stop
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl
> > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2):
> > +meter=1 pktps stats bands=
> > +type=drop rate=1
> > +
> > +meter=2 kbps stats bands=
> > +type=drop rate=1
> > +])
> > +
> > +ovs-appctl time/warp 5000
> > +for i in `seq 1 5`; do
> > +    AT_CHECK([ovs-appctl netdev-dummy/receive p7 \
> > +            'in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60])
> > +    AT_CHECK([ovs-appctl netdev-dummy/receive p8 \
> > +            'in_port(8),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3,dst=10.0.0.4,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60])
> > +done
> > +
> > +sleep 1  # wait for forwarders process packets
> > +
> > +# Meter 1 is measuring packets, allowing one packet per second,
> > +# so 4 out of 5 packets should hit the drop band.
> > +#
> > +# Meter 2 is measuring kbps (== 1000 bits). 2 packets
> > +# (120 bytes == 960 bits) pass, but the last 3 packets should hit the drop band.
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl
> > +OFPST_METER reply (OF1.3) (xid=0x2):
> > +meter:1 flow_count:1 packet_in_count:5 byte_in_count:300 duration:0.0s bands:
> > +0: packet_count:4 byte_count:240
> > +
> > +meter:2 flow_count:1 packet_in_count:5 byte_in_count:300 duration:0.0s bands:
> > +0: packet_count:3 byte_count:180
> > +])
> > +
> > +# Advance time by 1/2 second
> > +ovs-appctl time/warp 500
> > +
> > +for i in `seq 1 5`; do
> > +    AT_CHECK([ovs-appctl netdev-dummy/receive p7 \
> > +            'in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60])
> > +
> > +    AT_CHECK([ovs-appctl netdev-dummy/receive p8 \
> > +            'in_port(8),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3,dst=10.0.0.4,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)' --len 60])
> > +
> > +done
> > +
> > +sleep 1  # wait for forwarders process packets
> > +
> > +# Meter 1 is measuring packets, allowing one packet per second with
> > +# bursts of one packet, so all 5 of the new packets should hit the drop
>
> This comment still talks about bursts.
>
> It might make sense to not copy the whole test, but just add a couple of
> new ports and flows and one extra meter to the existing test.
> WDYT?
The original meter test case, use  "burst_size", if users don't use
it, OvS should work fine too. Because "burst_size" is important in the
merter function,
I use a new test case for it, that makes the test case clean.
> > +# band.
> > +# Meter 2 is measuring kbps (== 1000 bits). After 500ms
> > +# there should be space for 40 + 500 bits, so one new 60 byte (480 bit) packet
> > +# should pass, remaining 4 should hit the drop band.
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | strip_timers], [0], [dnl
> > +OFPST_METER reply (OF1.3) (xid=0x2):
> > +meter:1 flow_count:1 packet_in_count:10 byte_in_count:600 duration:0.0s bands:
> > +0: packet_count:9 byte_count:540
> > +
> > +meter:2 flow_count:1 packet_in_count:10 byte_in_count:600 duration:0.0s bands:
> > +0: packet_count:7 byte_count:420
> > +])
> > +
> > +ovs-appctl time/warp 5000
> > +
> > +AT_CHECK([
> > +ovs-appctl coverage/read-counter datapath_drop_meter
> > +], [0], [dnl
> > +16
> > +])
> > +
> > +AT_CHECK([cat ovs-vswitchd.log | filter_flow_install | strip_xout_keep_actions], [0], [dnl
> > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:meter(0),7
> > +recirc_id(0),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:8
> > +recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:meter(1),1
> > +recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:2
> > +])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> >  m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD],
> >    [AT_SETUP([dpif-netdev - partial hw offload - $1])
> >     OVS_VSWITCHD_START(
> >
>


-- 
Best regards, Tonghao


More information about the dev mailing list