[ovs-dev] [PATCH RESEND ovs v3 4/4] dpif: Don't set "burst_size" to "rate" if not specified.
Ilya Maximets
i.maximets at ovn.org
Tue Mar 30 21:16:48 UTC 2021
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?
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.
> /* 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?
> +# 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(
>
More information about the dev
mailing list