Re: [dpdk-dev] [PATCH v14 1/5] sched: add PIE based congestion management
From: Liguzinski, WojciechX <hidden>
Date: 2021-10-19 09:34:45
Hi Cristian, - As you asked I have added some points/one-liners in the patches what is changed - Regarding usage of rte_sched_cman_params structure I will write you a separate message - Reverted to use the RTE_SCHED_CMAN_RED flag - Also changed WRED occurrences back to RED - No, I wasn't getting a compilation error when struct rte_sched_cma_params was defined below its instantiation... But, to be certain I moved it above that place in code. - version.map updated with a comment, like you suggested Thanks for the review comments! BR, Wojtek -----Original Message----- From: Dumitrescu, Cristian <redacted> Sent: Friday, October 15, 2021 3:51 PM To: Liguzinski, WojciechX <redacted>; dev@dpdk.org; Singh, Jasvinder <redacted> Cc: Ajmera, Megha <redacted> Subject: RE: [PATCH v14 1/5] sched: add PIE based congestion management Hi Wojciech, Thank you for your patchset! Can you please, at least starting from this version, add a short change log at the top of your file, just after the signoff line? It helps a lot during the review process, and you can find abundant examples in other patchsets from this email list. One line of description for every change would be nice, thank you.
-----Original Message----- From: Liguzinski, WojciechX <redacted> Sent: Friday, October 15, 2021 9:16 AM To: dev@dpdk.org; Singh, Jasvinder <redacted>; Dumitrescu, Cristian [off-list ref] Cc: Ajmera, Megha <redacted> Subject: [PATCH v14 1/5] sched: add PIE based congestion management Implement PIE based congestion management based on rfc8033 Signed-off-by: Liguzinski, WojciechX <redacted> --- drivers/net/softnic/rte_eth_softnic_tm.c | 6 +- lib/sched/meson.build | 10 +- lib/sched/rte_pie.c | 82 +++++ lib/sched/rte_pie.h | 393 +++++++++++++++++++++++ lib/sched/rte_sched.c | 240 +++++++++----- lib/sched/rte_sched.h | 63 +++- lib/sched/version.map | 3 + 7 files changed, 701 insertions(+), 96 deletions(-) create mode 100644 lib/sched/rte_pie.c create mode 100644 lib/sched/rte_pie.h
<snip>
quoted hunk ↗ jump to hunk
diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c index a858f61f95..a066eed186 100644 --- a/lib/sched/rte_sched.c +++ b/lib/sched/rte_sched.c
<snip>
quoted hunk ↗ jump to hunk
@@ -183,8 +187,14 @@ struct rte_sched_subport { /* Pipe queues size */ uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; -#ifdef RTE_SCHED_RED - struct rte_red_configred_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][RTE_COLORS]; +#ifdef RTE_SCHED_CMAN + enum rte_sched_cman_mode cman; + + RTE_STD_C11 + union { + struct rte_red_config red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][RTE_COLORS]; + struct rte_pie_config pie_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; + }; #endif
Can you please use here the rte_sched_cman_params structure that you just created in rte_sched.h as opposed to inlining this structure. Yes, I agree it might have some ripple effect throughout this file, but I think it should be very limited, and also quick to do. <snip>
quoted hunk ↗ jump to hunk
diff --git a/lib/sched/rte_sched.h b/lib/sched/rte_sched.h index c1a772b70c..07fcf439d8 100644 --- a/lib/sched/rte_sched.h +++ b/lib/sched/rte_sched.h@@ -61,9 +61,10 @@ extern "C" { #include <rte_mbuf.h> #include <rte_meter.h> -/** Random Early Detection (RED) */ -#ifdef RTE_SCHED_RED +/** Congestion Management */ +#ifdef RTE_SCHED_CMAN #include "rte_red.h" +#include "rte_pie.h" #endif /** Maximum number of queues per pipe.@@ -110,6 +111,28 @@ extern "C" { #define RTE_SCHED_FRAME_OVERHEAD_DEFAULT 24 #endif +/** + * Congestion Management (CMAN) mode + * + * This is used for controlling the admission of packets into a packet queueor + * group of packet queues on congestion. + * + * The *Random Early Detection (RED)* algorithm works by proactively dropping + * more and more input packets as the queue occupancy builds up. When the queue + * is full or almost full, RED effectively works as *tail drop*. The *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 the + * latency around the target value. The congestion detection, however, is based + * on the queueing latency instead of the queue length like RED. For more + * information, refer RFC8033. + */ +enum rte_sched_cman_mode { + RTE_SCHED_CMAN_WRED, /**< Weighted Random Early Detection (WRED) */
The algorithm is RED, not WRED. Let's stick to RTE_SCHED_CMAN_RED, please. The Weighted aspect comes into place when defining the struct rte_sched_cman_params::red_params as an array indexed by color below.
quoted hunk ↗ jump to hunk
+ RTE_SCHED_CMAN_PIE, /**< Proportional Integral Controller Enhanced (PIE) */ +}; + /* * Pipe configuration parameters. The period and credits_per_period * parameters are measured in bytes, with one byte meaning the time@@ -174,12 +197,30 @@ struct rte_sched_subport_params { /** Max allowed profiles in the pipe profile table */ uint32_t n_max_pipe_profiles; -#ifdef RTE_SCHED_RED - /** RED parameters */ - struct rte_red_paramsred_params[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][RTE_COLORS]; +#ifdef RTE_SCHED_CMAN + /** Congestion Management parameters */ + struct rte_sched_cman_params *cman_params;
You are instantiating struct rte_sched_cma_params here, but you are defining it just below, i.e. after you attempted to instantiate it. Aren't you getting a build error when compiling with RTE_SCHED_CMAN defined?
#endif
};
+#ifdef RTE_SCHED_CMAN
+/*
+ * Congestion Management configuration parameters.
+ */
+struct rte_sched_cman_params {
+ /** Congestion Management mode */
+ enum rte_sched_cman_mode cman_mode;
+
+ union {
+ /** WRED parameters */In the comment: RED parameters instead of WRED, please.
+ struct rte_red_params red_params[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][RTE_COLORS]; + + /** PIE parameters */ + struct rte_pie_params pie_params[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; + }; +}; +#endif +
<snip>
quoted hunk ↗ jump to hunk
diff --git a/lib/sched/version.map b/lib/sched/version.map index 53c337b143..54e5e96d4f 100644 --- a/lib/sched/version.map +++ b/lib/sched/version.map@@ -30,4 +30,7 @@ EXPERIMENTAL { rte_sched_subport_pipe_profile_add; # added in 20.11 rte_sched_port_subport_profile_add; + + rte_pie_rt_data_init; + rte_pie_config_init;
You need to put a comment about the release when these symbols got introduced, similar to above.
}; -- 2.25.1
Thank you for your work! Regards, Cristian