Re: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management

["Morten Brørup" <mb@smartsharesystems.com>]

> 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 <wojciechx.liguzinski@intel.com>
> ---
>  drivers/net/softnic/rte_eth_softnic_tm.c |   4 +-
>  lib/sched/meson.build                    |  10 +-
>  lib/sched/rte_sched.c                    | 220 +++++++++++++++++------
>  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.

> +#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.

> -#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.
2. I suggest using "drops" as the variable name instead of "red" or "aqm".

> -#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.

> +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.

> diff --git a/lib/sched/rte_sched.h b/lib/sched/rte_sched.h
> 
> +/**
> + * Congestion management (CMAN) mode

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

> + *
> + * 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)
> */

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

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


> --------------------------------------------------------------
> Intel Research and Development Ireland Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> 
> 
> This e-mail and any attachments may contain confidential material for
> the sole
> use of the intended recipient(s). Any review or distribution by others
> is
> strictly prohibited. If you are not the intended recipient, please
> contact the
> sender and delete all copies.
> 

Please don't use this footer when sending to the DPDK mailing list.