Re: [dpdk-dev] [PATCH v14 1/5] sched: add PIE based congestion management

["Liguzinski, WojciechX" <wojciechx.liguzinski@intel.com>]

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 <cristian.dumitrescu@intel.com> 
Sent: Friday, October 15, 2021 3:51 PM
To: Liguzinski, WojciechX <wojciechx.liguzinski@intel.com>; dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>
Cc: Ajmera, Megha <megha.ajmera@intel.com>
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 <wojciechx.liguzinski@intel.com>
> Sent: Friday, October 15, 2021 9:16 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>; 
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Ajmera, Megha <megha.ajmera@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.liguzinski@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