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

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

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com> 
> Sent: Tuesday, May 25, 2021 11:17 AM
> To: Liguzinski, WojciechX <wojciechx.liguzinski@intel.com>; dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Dharmappa, Savinay <savinay.dharmappa@intel.com>
> Subject: RE: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management
>
> > 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.

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

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

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

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

Ok, I will change that.

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

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

>
> > 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. ;-)

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

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.

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

Footer issue has been handled.

Thanks,
Wojtek