[ovs-dev] [PATCH] dpif-netdev: Remove meter rate from the bucket size calculation.
Tonghao Zhang
xiangxia.m.yue at gmail.com
Wed May 12 12:13:19 UTC 2021
On Wed, Apr 21, 2021 at 9:48 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> 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>
Reviewed-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
Thanks!
> ---
> 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
>
--
Best regards, Tonghao
More information about the dev
mailing list