On Wed, Mar 31, 2021 at 5:16 AM Ilya Maximets <[email protected]> wrote: > > On 3/3/21 3:46 PM, [email protected] wrote: > > From: Tonghao Zhang <[email protected]> > > > > 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 <[email protected]> > > --- > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
