[ovs-dev] [PATCH] dpif-netdev: Remove meter rate from the bucket size calculation.
Ilya Maximets
i.maximets at ovn.org
Wed Apr 21 13:48:16 UTC 2021
Implementation of meters supposed to be a classic token bucket with 2
typical parameters: rate and burst size.
Burst size in this schema is the maximum number of bytes/packets that
could pass without being rate limited.
Recent changes to userspace datapath made meter implementation to be
in line with the kernel one, and this uncovered several issues.
The main problem is that maximum bucket size for unknown reason
accounts not only burst size, but also the numerical value of rate.
This creates a lot of confusion around behavior of meters.
For example, if rate is configured as 1000 pps and burst size set to 1,
this should mean that meter will tolerate bursts of 1 packet at most,
i.e. not a single packet above the rate should pass the meter.
However, current implementation calculates maximum bucket size as
(rate + burst size), so the effective bucket size will be 1001. This
means that first 1000 packets will not be rate limited and average
rate might be twice as high as the configured rate. This also makes
it practically impossible to configure meter that will have burst size
lower than the rate, which might be a desirable configuration if the
rate is high.
Inability to configure low values of a burst size and overall inability
for a user to predict what will be a maximum and average rate from the
configured parameters of a meter without looking at the OVS and kernel
code might be also classified as a security issue, because drop meters
are frequently used as a way of protection from DoS attacks.
This change removes rate from the calculation of a bucket size, making
it in line with the classic token bucket algorithm and essentially
making the rate and burst tolerance being predictable from a users'
perspective.
Same change will be proposed for the kernel implementation.
Unit tests changed back to their correct version and enhanced.
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
---
lib/dpif-netdev.c | 5 ++--
tests/dpif-netdev.at | 53 +++++++++++++++++++++++++++++++------------
tests/ofproto-dpif.at | 2 +-
3 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 251788b04..650e67ab3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6229,7 +6229,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
uint64_t max_bucket_size;
band = &meter->bands[m];
- max_bucket_size = (band->rate + band->burst_size) * 1000ULL;
+ max_bucket_size = band->burst_size * 1000ULL;
/* Update band's bucket. */
band->bucket += (uint64_t) delta_t * band->rate;
if (band->bucket > max_bucket_size) {
@@ -6357,8 +6357,7 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
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;
+ meter->bands[i].bucket = meter->bands[i].burst_size * 1000ULL;
/* Figure out max delta_t that is enough to fill any bucket. */
band_max_delta_t
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 57cae383f..16402ebae 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -314,21 +314,21 @@ done
sleep 1 # wait for forwarders process packets
# Meter 1 is measuring packets, allowing one packet per second with
-# bursts of one packet, so 3 out of 5 packets should hit the drop
-# band.
-# Meter 2 is measuring kbps, with burst size 2 (== 3000 bits). 6 packets
-# (360 bytes == 2880 bits) pass, but the last packet should hit the drop band.
+# bursts of one packet, so 4 out of 5 packets should hit the drop band.
+# Meter 2 is measuring kbps, with burst size 2 (== 2000 bits). 4 packets
+# (240 bytes == 1920 bits) pass, but the last three packets should hit the
+# drop band. There should be 80 bits remaining for the next packets.
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:3 byte_count:180
+0: packet_count:4 byte_count:240
meter:2 flow_count:1 packet_in_count:7 byte_in_count:420 duration:0.0s bands:
-0: packet_count:1 byte_count:60
+0: packet_count:3 byte_count:180
])
-# Advance time by 1/2 second
-ovs-appctl time/warp 500
+# Advance time by 870 ms
+ovs-appctl time/warp 870
for i in `seq 1 5`; do
AT_CHECK(
@@ -345,16 +345,41 @@ 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
# band.
-# Meter 2 is measuring kbps, with burst size 2 (== 3000 bits). After 500ms
-# there should be space for 120 + 500 bits, so one new 60 byte (480 bit) packet
-# should pass, remaining 4 should hit the drop band.
+# Meter 2 is measuring kbps, with burst size 2 (== 2000 bits). After 870ms
+# there should be space for 80 + 870 = 950 bits, so one new 60 byte (480 bit)
+# packet should pass, remaining 4 should hit the drop band. There should be
+# 470 bits left.
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:8 byte_count:480
+0: packet_count:9 byte_count:540
meter:2 flow_count:1 packet_in_count:12 byte_in_count:720 duration:0.0s bands:
-0: packet_count:5 byte_count:300
+0: packet_count:7 byte_count:420
+])
+
+# Advance time by 10 ms
+ovs-appctl time/warp 10
+
+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])
+done
+
+sleep 1 # wait for forwarders process packets
+
+# Meter 1 should remain the same as we didn't send anything that should hit it.
+# Meter 2 is measuring kbps, with burst size 2 (== 2000 bits). After 10ms
+# there should be space for 470 + 10 = 480 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:17 byte_in_count:1020 duration:0.0s bands:
+0: packet_count:11 byte_count:660
])
ovs-appctl time/warp 5000
@@ -362,7 +387,7 @@ ovs-appctl time/warp 5000
AT_CHECK([
ovs-appctl coverage/read-counter datapath_drop_meter
], [0], [dnl
-13
+20
])
AT_CHECK([cat ovs-vswitchd.log | filter_flow_install | strip_xout_keep_actions], [0], [dnl
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 24bbd884c..31064ed95 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2123,7 +2123,7 @@ AT_CHECK([ovs-appctl revalidator/purge])
AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log])
dnl Add a controller meter.
-AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=controller pktps burst stats bands=type=drop rate=1 burst_size=1'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=controller pktps stats bands=type=drop rate=2'])
dnl Advance time by 1 second.
AT_CHECK([ovs-appctl time/warp 1000], [0], [ignore])
--
2.26.3
More information about the dev
mailing list