From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liguzinski,
WojciechX
Sent: Wednesday, 9 June 2021 10.37

quoted

From: Morten Brørup <redacted>
Sent: Tuesday, May 25, 2021 11:17 AM

quoted

From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liguzinski,
WojciechX
Sent: Monday, 24 May 2021 12.58

Implement pie based congestion management based on rfc8033

Signed-off-by: Liguzinski, WojciechX

[off-list ref]

quoted

quoted

---
 drivers/net/softnic/rte_eth_softnic_tm.c |   4 +-
 lib/sched/meson.build                    |  10 +-
 lib/sched/rte_sched.c                    | 220 +++++++++++++++++--

----

quoted

quoted

 lib/sched/rte_sched.h                    |  53 ++++--
 4 files changed, 210 insertions(+), 77 deletions(-)

Please use the abbreviation AQM instead of CMAN in the source code.

This applies to the RTE_SCHED_CMAN definition, as well as functions,
enums and variable names.

Ok, sure, I'm going to change that where applicable.

quoted

quoted

+#ifdef RTE_SCHED_CMAN
+
+static int
+rte_sched_red_config (struct rte_sched_port *port,
+	struct rte_sched_subport *s,
+	struct rte_sched_subport_params *params,
+	uint32_t n_subports)
+{
+	uint32_t i;
+
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+
+		uint32_t j;
+
+		for (j = 0; j < RTE_COLORS; j++) {
+			/* if min/max are both zero, then RED is disabled */
+			if ((params->red_params[i][j].min_th |
+				 params->red_params[i][j].max_th) == 0) {
+				continue;
+			}
+
+			if (rte_red_config_init(&s->red_config[i][j],
+				params->red_params[i][j].wq_log2,
+				params->red_params[i][j].min_th,
+				params->red_params[i][j].max_th,
+				params->red_params[i][j].maxp_inv) != 0) {
+				rte_sched_free_memory(port, n_subports);
+
+				RTE_LOG(NOTICE, SCHED,
+				"%s: RED configuration init fails\n",
__func__);
+				return -EINVAL;
+			}
+		}
+	}
+	s->cman = RTE_SCHED_CMAN_WRED;
+	return 0;
+}
+
+static int
+rte_sched_pie_config (struct rte_sched_port *port,
+	struct rte_sched_subport *s,
+	struct rte_sched_subport_params *params,
+	uint32_t n_subports)
+{
+	uint32_t i;
+
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+		if (params->pie_params[i].tailq_th > params->qsize[i]) {
+			RTE_LOG(NOTICE, SCHED,
+			"%s: PIE tailq threshold incorrect \n", __func__);
+			return -EINVAL;
+		}
+
+		if (rte_pie_config_init(&s->pie_config[i],
+			params->pie_params[i].qdelay_ref,
+			params->pie_params[i].dp_update_interval,
+			params->pie_params[i].max_burst,
+			params->pie_params[i].tailq_th) != 0) {
+			rte_sched_free_memory(port, n_subports);
+
+			RTE_LOG(NOTICE, SCHED,
+			"%s: PIE configuration init fails\n", __func__);
+			return -EINVAL;
+			}
+	}
+	s->cman = RTE_SCHED_CMAN_PIE;
+	return 0;
+}

I suggest moving the two above functions from rte_sched.c to

respectively rte_red.c and rte_pie.c.

rte_red.c and rte_pie.c hold functions implementing those algorithms
and they don't know anything about ports and subports. That part refers
to scheduler implementation. Putting those methods respectively to
those files would in my opinion break the 'functional isolation'.

Then it makes sense keeping them here. You can ignore my suggestion.

quoted

quoted

-#ifdef RTE_SCHED_RED
+#ifdef RTE_SCHED_CMAN
 static inline void
 rte_sched_port_update_subport_stats_on_drop(struct rte_sched_port
*port,
 	struct rte_sched_subport *subport,
 	uint32_t qindex,
 	struct rte_mbuf *pkt,
-	uint32_t red)
+	uint32_t cman)
 #else
 static inline void
 rte_sched_port_update_subport_stats_on_drop(struct rte_sched_port
*port,
 	struct rte_sched_subport *subport,
 	uint32_t qindex,
 	struct rte_mbuf *pkt,
-	__rte_unused uint32_t red)
+	__rte_unused uint32_t cman)
 #endif

Two comments:
1. __rte_unused indicates that the variable might be unused, not that

it is never used. So you do not need the first variant of this function
declaration.

Thanks, it's going to be fixed.

quoted

2. I suggest using "drops" as the variable name instead of "red" or

"aqm".

Ok, I will change that.

quoted

quoted

-#ifdef RTE_SCHED_RED
+#ifdef RTE_SCHED_CMAN
 static inline void
 rte_sched_port_update_queue_stats_on_drop(struct rte_sched_subport
*subport,
 	uint32_t qindex,
 	struct rte_mbuf *pkt,
-	uint32_t red)
+	uint32_t cman)
 #else
 static inline void
 rte_sched_port_update_queue_stats_on_drop(struct rte_sched_subport
*subport,
 	uint32_t qindex,
 	struct rte_mbuf *pkt,
-	__rte_unused uint32_t red)
+	__rte_unused uint32_t cman)
 #endif

The above two comments also apply here.

Ok, it's going to be changed.

quoted

quoted

+static inline void
+rte_sched_port_pie_dequeue(struct rte_sched_subport *subport,
+uint32_t qindex, uint32_t pkt_len, uint64_t time) {
+	struct rte_sched_queue_extra *qe = subport->queue_extra + qindex;
+	struct rte_pie *pie = &qe->pie;
+
+	/* Update queue length */
+	pie->qlen -= 1;
+	pie->qlen_bytes -= pkt_len;
+
+	rte_pie_dequeue (pie, pkt_len, time);
 }

Can the RED/PIE specific functions somehow move to rte_red.c and

rte_pie.c without degrading performance? Perhaps function pointers are
required. This prevents rte_sched.c from growing too much.

Like I mentioned above, those functions use data structures known to
scheduler and not directly to those algorithms which are implemented in
those definition files. I will try think of a solution that could be
suitable here.

Now that I understand your line of thinking, I agree with you. You can ignore my comment here too.

quoted

quoted

diff --git a/lib/sched/rte_sched.h b/lib/sched/rte_sched.h

+/**
+ * Congestion management (CMAN) mode

"Active Queue Management (AQM) mode", please.

Sure. ;-)

quoted

quoted

+ *
+ * This is used for controlling the admission of packets into a
+ packet
queue or
+ * group of packet queues on congestion.
+ *
+ * The *Random Early Detection (RED)* algorithm works by

proactively

quoted

quoted

dropping
+ * more and more input packets as the queue occupancy builds up.

When

quoted

quoted

the queue
+ * is full or almost full, RED effectively works as *tail drop*.

The

quoted

quoted

*Weighted
+ * RED* algorithm uses a separate set of RED thresholds for each
packet color.
+ *
+ * Similar to RED, Proportional Integral Controller Enhanced (PIE)
randomly
+ * drops a packet at the onset of the congestion and tries to

control

quoted

quoted

the
+ * latency around the target value. The congestion detection,
+ however,
is based
+ * on the queueing latency instead of the queue length like RED.

For

quoted

quoted

more
+ * information, refer RFC8033.
+ */
+enum rte_sched_cman_mode {
+	RTE_SCHED_CMAN_WRED, /**< Weighted Random Early Detection (WRED)
*/

Please stick with either the name RED or WRED, for consistency.

WRED is just an extension of RED so in places where I found that it is
suitable I have used such naming, otherwise RED. I think it shouldn't
be changed in all places as it may be confusing.

I don't have a strong opinion about this, and you are putting some thoughts into it, so I'm happy with that.

quoted

quoted

+	RTE_SCHED_CMAN_PIE,  /**< Proportional Integral Controller
Enhanced (PIE) */
+};
+

[snip]

Footer issue has been handled.

Thanks,
Wojtek

:-)