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 <wojciechx.liguzin...@intel.com>
> Sent: Friday, October 15, 2021 9:16 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.si...@intel.com>;
> Dumitrescu, Cristian <cristian.dumitre...@intel.com>
> Cc: Ajmera, Megha <megha.ajm...@intel.com>
> Subject: [PATCH v14 1/5] sched: add PIE based congestion management
>
> Implement PIE based congestion management based on rfc8033
>
> Signed-off-by: Liguzinski, WojciechX <wojciechx.liguzin...@intel.com>
> ---
> 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>
> 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>
> @@ -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_config
> red_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>
> 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 queue
> or
> + * 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.
> + 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_params
> red_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>
> 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
