[ovs-dev] [PATCH ovs v3 2/4] dpif-netdev: Refactor the buckets calculation.

[xiangxia.m.yue at gmail.com]

From: Tonghao Zhang <xiangxia.m.yue at gmail.com>

The way that "burst_size" was used as total buckets is
very strange. If user set the "burst_size" too smaller while
"rate" larger, that may affect the meter normal work.
This patch refactor the buckets calculation, and start
with a full buckets.

Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
---
 lib/dpif-netdev.c    | 17 ++++++++++-------
 tests/dpif-netdev.at | 14 +++++++-------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 78f3eef5381b..77df376a26bc 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -280,8 +280,8 @@ static bool dpcls_lookup(struct dpcls *cls,
 
 struct dp_meter_band {
     uint32_t rate;
+    uint32_t burst_size;
     uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for KBPS) */
-    uint64_t burst_size;
     uint64_t packet_count;
     uint64_t byte_count;
 };
@@ -6204,12 +6204,14 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
     /* Update all bands and find the one hit with the highest rate for each
      * packet (if any). */
     for (int m = 0; m < meter->n_bands; ++m) {
-        band = &meter->bands[m];
+        uint64_t max_bucket_size;
 
+        band = &meter->bands[m];
+        max_bucket_size = band->rate * 1000ULL;
         /* Update band's bucket. */
         band->bucket += (uint64_t) delta_t * band->rate;
-        if (band->bucket > band->burst_size) {
-            band->bucket = band->burst_size;
+        if (band->bucket > max_bucket_size) {
+            band->bucket = max_bucket_size;
         }
 
         /* Drain the bucket for all the packets, if possible. */
@@ -6330,13 +6332,14 @@ dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
             config->bands[i].burst_size = config->bands[i].rate;
         }
 
-        meter->bands[i].bucket = 0;
         meter->bands[i].rate = config->bands[i].rate;
-        meter->bands[i].burst_size = config->bands[i].burst_size * 1000ULL;
+        meter->bands[i].burst_size = config->bands[i].burst_size;
+        /* Start with a full bucket. */
+        meter->bands[i].bucket = meter->bands[i].rate * 1000ULL;
 
         /* Figure out max delta_t that is enough to fill any bucket. */
         band_max_delta_t
-            = meter->bands[i].burst_size / meter->bands[i].rate;
+            = meter->bands[i].bucket / meter->bands[i].rate;
         if (band_max_delta_t > meter->max_delta_t) {
             meter->max_delta_t = band_max_delta_t;
         }
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 2862a3c9b96d..a30d026cabe1 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -314,15 +314,15 @@ sleep 1  # wait for forwarders process packets
 # Meter 1 is measuring packets, allowing one packet per second with
 # 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 packet should hit the drop band.
+# Meter 2 is measuring kbps, with burst size 2 (== 1000 bits). 2 packets
+# (120 bytes == 960 bits) pass, but the last packet 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:1 byte_count:60
+0: packet_count:3 byte_count:180
 ])
 
 # Advance time by 1/2 second
@@ -343,8 +343,8 @@ 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 (== 2000 bits). After 500ms
-# there should be space for 80 + 500 bits, so one new 60 byte (480 bit) packet
+# Meter 2 is measuring kbps, with burst size 2 (== 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):
@@ -352,7 +352,7 @@ 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:5 byte_count:300
+0: packet_count:7 byte_count:420
 ])
 
 ovs-appctl time/warp 5000
@@ -360,7 +360,7 @@ ovs-appctl time/warp 5000
 AT_CHECK([
 ovs-appctl coverage/read-counter datapath_drop_meter
 ], [0], [dnl
-14
+16
 ])
 
 AT_CHECK([cat ovs-vswitchd.log | filter_flow_install | strip_xout_keep_actions], [0], [dnl
-- 
2.27.0