Fix simple_switch CLI commands to configure queues (#1276)

The queue id for the `set_queue_rate` and `set_queue_depth` commands
did not match the "datapath" queue id, as used in the P4 program.

This patch fixes the inconsistency by changing the priority queue implementation
so that the queue with the highest numerical priority is served first. For the P4
programmer, the observable behavior is not changed: queue with priority 0 is
the lowest priority queue. However, the CLI commands now work as expected.

Fixes #1264 

Signed-off-by: Rizky Ramadhana <rizkyramadhana26@gmail.com>

docs/runtime_CLI.md

@@ -469,11 +469,35 @@ TODO: reset_state
 TODO: serialize_state
 TODO: set_crc16_parameters
 TODO: set_crc32_parameters
-TODO: set_queue_depth [simple_switch_CLI only]
-TODO: set_queue_rate [simple_switch_CLI only]
 TODO: shell
 ```
 
+### set_queue_depth [simple_switch_CLI only]
+```
+RuntimeCmd: help set_queue_depth
+Set depth of one / all egress queue(s): set_queue_depth <nb_pkts> [<egress_port> [<priority>]]
+```
+
+This command sets the maximum depth of queue(s) to `nb_pkts` in units of 
+number of packets. The affected priority queue is identified by `egress_port` 
+and `priority`. If `priority` is not given, this command sets the maximum depth 
+for all queues in the given `egress_port`. If both `priority` and 
+`egress_port` are not given, this command sets the maximum depth for all queues. 
+The `priority` mentioned here is the same as qid in the queueing_metadata.
+
+### set_queue_rate [simple_switch_CLI only]
+```
+RuntimeCmd: help set_queue_rate
+Set rate of one / all egress queue(s): set_queue_rate <rate_pps> [<egress_port> [<priority>]]
+```
+
+This command sets the rate at which queue(s) is(are) consumed to `rate_pps`, 
+in units of packet per second. The affected priority queue is identified by 
+`egress_port` and `priority`. If `priority` is not given, this command sets 
+the rate for all queues in the given `egress_port`. If both `priority` and 
+`egress_port` are not given, this command sets the rate for all queues. 
+The `priority` mentioned here is the same as qid in the queueing_metadata.
+
 ### show_actions
 
 No parameters.  For every action in the currently loaded P4 program, shows the

docs/simple_switch.md

@@ -223,7 +223,13 @@ queue.
 - `deq_qdepth`: the depth of queue when the packet was dequeued, in units of number of packets (not the total size of packets).
 - `qid`: when there are multiple queues servicing each egress port (e.g. when
 priority queueing is enabled), each queue is assigned a fixed unique id, which
-is written to this field. Otherwise, this field is set to 0.
+is written to this field. Otherwise, this field is set to 0. If 
+priority queueing is enabled, the qid also describes the priority level 
+of each queue. Starting with 0 that has the lowest priority until 
+`number_of_priority_queues - 1` that has the highest priority. The 
+number of priority queues for each port can be defined by adding 
+`--priority-queues` when running `simple_switch`.
+
 TBD: `qid` is not currently part of type `standard_metadata_t` in v1model.
 Perhaps it should be added?
 

include/bm/bm_sim/queueing.h

@@ -473,13 +473,13 @@ class QueueingLogicRL {
 //! This class is slightly more advanced than QueueingLogicRL. The difference
 //! between the 2 is that this one offers the ability to set several priority
 //! queues for each logical queue. Priority queues are numbered from `0` to
-//! `nb_priorities` (see QueueingLogicPriRL::QueueingLogicPriRL()). Priority `0`
-//! is the highest priority queue. Each priority queue can have its own rate and
-//! its own capacity. Queues will be served in order of priority, until their
-//! respective maximum rate is reached. If no maximum rate is set, queues with a
-//! high priority can starve lower-priority queues. For example, if the queue
-//! with priority `0` always contains at least one element, the other queues
-//! will never be served.
+//! `nb_priorities-1` (see QueueingLogicPriRL::QueueingLogicPriRL()). Priority
+//! `0` is the lowest priority queue. Each priority queue can have its own rate
+//! and its own capacity. Queues will be served in order of priority, until
+//! their respective maximum rate is reached. If no maximum rate is set, queues
+//! with a high priority can starve lower-priority queues. For example, if
+//! the queue with priority `nb_priorities - 1` always contains at least one
+//! element, the other queues will never be served.
 //! As for QueueingLogicRL, the write behavior (push_front()) is not blocking:
 //! once a logical queue is full, subsequent incoming elements will be dropped
 //! until the queue starts draining again.
@@ -562,7 +562,7 @@ class QueueingLogicPriRL {
   //! element is copied to \p queue_id and the priority value of the served
   //! queue is copied to \p priority.
   //! Elements are retrieved according to the priority queue they are in
-  //! (highest priorities, i.e. lowest priority values, are served first). Once
+  //! (highest priority values are served first). Once
   //! a given priority queue reaches its maximum rate, the next queue is served.
   //! If no elements are available (either the queues are empty or they have
   //! exceeded their rate already), the function will block.
@@ -578,7 +578,8 @@ class QueueingLogicPriRL {
       } else {
         auto now = clock::now();
         auto next = clock::time_point::max();
-        for (pri = 0; pri < nb_priorities; pri++) {
+        // This will iterate from nb_priorities-1 to 0
+        for (pri = nb_priorities ; pri-- > 0;) {
           auto &q = w_info.queues[pri];
           if (q.size() == 0) continue;
           if (q.top().send <= now) {

targets/simple_switch/simple_switch.cpp

@@ -420,7 +420,7 @@ SimpleSwitch::enqueue(port_t egress_port, std::unique_ptr<Packet> &&packet) {
     }
 
     egress_buffers.push_front(
-        egress_port, nb_queues_per_port - 1 - priority,
+        egress_port, priority,
         std::move(packet));
 }
 
@@ -671,7 +671,7 @@ SimpleSwitch::egress_thread(size_t worker_id) {
           egress_buffers.size(port, priority));
       if (phv->has_field("queueing_metadata.qid")) {
         auto &qid_f = phv->get_field("queueing_metadata.qid");
-        qid_f.set(nb_queues_per_port - 1 - priority);
+        qid_f.set(priority);
       }
     }
 

tests/test_queueing.cpp

@@ -292,14 +292,14 @@ TEST_F(QueueingPriRLTest, Pri) {
   // if removed, g++ complains that capacity was not defined
   const size_t c = capacity;
 
-  // we have to receive at least capacity P1 elements (they are buffered and
+  // we have to receive at least capacity P0 elements (they are buffered and
   // dequeued at the end...)
-  ASSERT_LT(c, priority_1);
+  ASSERT_LT(c, priority_0);
 
-  // most elements should be P0
+  // most elements should be P1
   // was originally 10%, but replaced it with 20% as the test would fail from
   // time to time
-  ASSERT_GT(0.2, (priority_1 - c) / static_cast<double>(priority_0));
+  ASSERT_GT(0.2, (priority_0 - c) / static_cast<double>(priority_1));
 }
 
 TEST_F(QueueingPriRLTest, PriRateLimiter) {