Correct the unit of max_burst parameter for ratelimit TCs (#470)

* Correct the unit of max_burst: work/time -> work

* Show max_burst on "show tc"

* Add a burstiness example for rate_limit TC

* Update unittest

Author: sangjinhan

bessctl/commands.py

@@ -908,36 +908,55 @@ def show_worker_list(cli, worker_ids):
 
 
 def _limit_to_str(limit):
-    buf = []
-
     if 'count' in limit:
-        buf.append('%d times' % limit['count'])
-
-    if 'cycle' in limit:
-        buf.append('%.3f Mhz' % (limit['cycle'] / 1e6))
-
-    if 'packet' in limit:
+        return '%d times/s' % limit['count']
+    elif 'cycle' in limit:
+        return '%.3f MHz' % (limit['cycle'] / 1e6)
+    elif 'packet' in limit:
         if limit['packet'] < 1e3:
-            buf.append('%.d pps' % limit['packet'])
+            return '%.d pps' % limit['packet']
         elif limit['packet'] < 1e6:
-            buf.append('%.3f kpps' % (limit['packet'] / 1e3))
+            return '%.3f kpps' % (limit['packet'] / 1e3)
         else:
-            buf.append('%.3f Mpps' % (limit['packet'] / 1e6))
-
-    if 'bit' in limit:
+            return '%.3f Mpps' % (limit['packet'] / 1e6)
+    elif 'bit' in limit:
         if limit['bit'] < 1e3:
-            buf.append('%.d bps' % limit['bit'])
+            return '%.d bps' % limit['bit']
         elif limit['bit'] < 1e6:
-            buf.append('%.3f kbps' % (limit['bit'] / 1e3))
+            return '%.3f kbps' % (limit['bit'] / 1e3)
         else:
-            buf.append('%.3f Mbps' % (limit['bit'] / 1e6))
-
-    if buf:
-        return 'limits: ' + ', '.join(buf)
+            return '%.3f Mbps' % (limit['bit'] / 1e6)
     else:
         return 'unlimited'
 
 
+def _burst_to_str(burst):
+    # no output if max_burst is not set
+    if len(burst.values()) == 0 or burst.values()[0] == 0:
+        return ''
+
+    if 'count' in burst:
+        return 'burst: %d times' % burst['count']
+    elif 'cycle' in burst:
+        return 'burst: %d cycles' % burst['cycle']
+    elif 'packet' in burst:
+        if burst['packet'] < 1e3:
+            return 'burst: %.d pkts' % burst['packet']
+        elif burst['packet'] < 1e6:
+            return 'burst: %.3f kpkts' % (burst['packet'] / 1e3)
+        else:
+            return 'burst: %.3f Mpkts' % (burst['packet'] / 1e6)
+    elif 'bit' in burst:
+        if burst['bit'] < 1e3:
+            return 'burst: %.d bits' % burst['bit']
+        elif burst['bit'] < 1e6:
+            return 'burst: %.3f kbits' % (burst['bit'] / 1e3)
+        else:
+            return 'burst: %.3f Mbits' % (burst['bit'] / 1e6)
+    else:
+        return ''
+
+
 def _show_tcs_node(cli, node, indent, prefix, lastsibling):
     line = prefix
     if indent > 0:
@@ -1010,6 +1029,7 @@ def _build_tcs_tree(tcs):
 
         if c_.policy == "rate_limit":
             nodes[c_.name]["show_list"].append(_limit_to_str(c_.limit))
+            nodes[c_.name]["show_list"].append(_burst_to_str(c_.max_burst))
 
     return root
 

bessctl/conf/samples/tc/max_burst.bess

@@ -0,0 +1,35 @@
+# Compare worker 0 and worker 1 with "monitor tc"
+
+bess.add_worker(wid=0, core=0)
+bess.add_worker(wid=1, core=1)
+dummy_pkt = '0' * 100
+
+# Worker 0: no burstiness causes inaccurate scheduling
+w0_src0::Source() -> w0_rewrite::Rewrite(templates=[dummy_pkt]) -> Sink()
+w0_src1::Source() -> w0_rewrite
+
+w0_src0.set_burst(burst=1)
+w0_src1.set_burst(burst=32)
+
+bess.add_tc('w0_1000MHz', policy='rate_limit', resource='cycle', limit={'cycle': int(1e9)}, wid=0)
+bess.add_tc('w0_rr', policy='round_robin', parent='w0_1000MHz')
+bess.add_tc('w0_500MHz_0', policy='rate_limit', resource='cycle', limit={'cycle': int(0.5e9)}, parent='w0_rr')
+bess.add_tc('w0_500MHz_1', policy='rate_limit', resource='cycle', limit={'cycle': int(0.5e9)}, parent='w0_rr')
+
+bess.attach_module(w0_src0.name, 'w0_500MHz_0')
+bess.attach_module(w0_src1.name, 'w0_500MHz_1')
+
+# Worker 1: traffic classes with burstiness allowance
+w1_src0::Source() -> w1_rewrite::Rewrite(templates=[dummy_pkt]) -> Sink()
+w1_src1::Source() -> w1_rewrite
+
+w1_src0.set_burst(burst=1)
+w1_src1.set_burst(burst=32)
+
+bess.add_tc('w1_1000MHz', policy='rate_limit', resource='cycle', limit={'cycle': int(1e9)}, wid=1)
+bess.add_tc('w1_rr', policy='round_robin', parent='w1_1000MHz')
+bess.add_tc('w1_500MHz_0', policy='rate_limit', resource='cycle', limit={'cycle': int(0.5e9)}, max_burst={'cycle': 10000}, parent='w1_rr')
+bess.add_tc('w1_500MHz_1', policy='rate_limit', resource='cycle', limit={'cycle': int(0.5e9)}, max_burst={'cycle': 10000}, parent='w1_rr')
+
+bess.attach_module(w1_src0.name, 'w1_500MHz_0')
+bess.attach_module(w1_src1.name, 'w1_500MHz_1')

core/traffic_class.cc

@@ -465,7 +465,7 @@ void RateLimitTrafficClass::FinishAndAccountTowardsRoot(
   last_tsc_ = tsc;
 
   uint64_t tokens = tokens_ + limit_ * elapsed_cycles;
-  uint64_t consumed = usage[resource_] << USAGE_AMPLIFIER_POW;
+  uint64_t consumed = to_work_units(usage[resource_]);
   if (tokens < consumed) {
     // Exceeded limit, throttled.
     tokens_ = 0;

core/traffic_class.h

@@ -438,12 +438,8 @@ class RateLimitTrafficClass final : public TrafficClass {
         tokens_(),
         last_tsc_(),
         child_() {
-    limit_arg_ = limit;
-    limit_ = to_work_units(limit);
-    if (limit_) {
-      max_burst_arg_ = max_burst;
-      max_burst_ = to_work_units(max_burst);
-    }
+    set_limit(limit);
+    set_max_burst(max_burst);
   }
 
   ~RateLimitTrafficClass();
@@ -482,7 +478,7 @@ class RateLimitTrafficClass final : public TrafficClass {
   // Set the limit to `limit`, which is in units of the resource type
   void set_limit(uint64_t limit) {
     limit_arg_ = limit;
-    limit_ = to_work_units(limit);
+    limit_ = to_work_units_per_cycle(limit);
   }
 
   // Set the max burst to `burst`, which is in units of the resource type
@@ -495,9 +491,14 @@ class RateLimitTrafficClass final : public TrafficClass {
 
   void TraverseChildren(std::function<void(TCChildArgs *)>) const override;
 
+  // Convert resource units to work units per cycle
+  static uint64_t to_work_units_per_cycle(uint64_t x) {
+    return (x << (USAGE_AMPLIFIER_POW - 4)) / (tsc_hz >> 4);
+  }
+
   // Convert resource units to work units
   static uint64_t to_work_units(uint64_t x) {
-    return (x << (USAGE_AMPLIFIER_POW - 4)) / (tsc_hz >> 4);
+    return x << USAGE_AMPLIFIER_POW;
   }
 
  private:
@@ -516,8 +517,8 @@ class RateLimitTrafficClass final : public TrafficClass {
   // tb->limit < 2^36
   uint64_t limit_;          // In work units per cycle (0 if unlimited).
   uint64_t limit_arg_;      // In resource units per second.
-  uint64_t max_burst_;      // In work units per cycle (0 if unlimited).
-  uint64_t max_burst_arg_;  // In resource units per second.
+  uint64_t max_burst_;      // In work units.
+  uint64_t max_burst_arg_;  // In resource units.
   uint64_t tokens_;         // In work units.
 
   // Last time this TC was scheduled.

core/traffic_class_test.cc

@@ -277,7 +277,7 @@ TEST(DefaultSchedulerNext, BasicTreeRateLimit) {
 
   ASSERT_EQ(RESOURCE_COUNT, c->resource());
   ASSERT_EQ(50, c->limit_arg());
-  ASSERT_EQ(RateLimitTrafficClass::to_work_units(50), c->limit());
+  ASSERT_EQ(RateLimitTrafficClass::to_work_units_per_cycle(50), c->limit());
   ASSERT_EQ(100, c->max_burst_arg());
   ASSERT_EQ(RateLimitTrafficClass::to_work_units(100), c->max_burst());
 
@@ -287,7 +287,8 @@ TEST(DefaultSchedulerNext, BasicTreeRateLimit) {
 
   ASSERT_EQ(RESOURCE_PACKET, c->resource());
   ASSERT_EQ(new_limit, c->limit_arg());
-  ASSERT_EQ(RateLimitTrafficClass::to_work_units(new_limit), c->limit());
+  ASSERT_EQ(RateLimitTrafficClass::to_work_units_per_cycle(new_limit),
+            c->limit());
   ASSERT_EQ(new_burst, c->max_burst_arg());
   ASSERT_EQ(RateLimitTrafficClass::to_work_units(new_burst), c->max_burst());