DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Liguzinski, WojciechX" <wojciechx.liguzinski@intel.com>
To: Morten Brørup <mb@smartsharesystems.com>,
	"Singh, Jasvinder" <jasvinder.singh@intel.com>,
	"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Cc: "Dharmappa, Savinay" <savinay.dharmappa@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Ajmera, Megha" <megha.ajmera@intel.com>
Subject: Re: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management
Date: Wed, 9 Jun 2021 08:36:32 +0000
Message-ID: <MWHPR1101MB2112082D4CEB33C01604E6BF94369@MWHPR1101MB2112.namprd11.prod.outlook.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C617C3@smartserver.smartshare.dk>


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

  reply	other threads:[~2021-06-09  8:37 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-24 10:58 [dpdk-dev] [RFC PATCH 0/3] Add PIE support for HQoS library Liguzinski, WojciechX
2021-05-24 10:58 ` [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management Liguzinski, WojciechX
2021-05-25  9:16   ` Morten Brørup
2021-06-09  8:36     ` Liguzinski, WojciechX [this message]
2021-06-09 12:35       ` Morten Brørup
2021-05-24 10:58 ` [dpdk-dev] [RFC PATCH 2/3] example/qos_sched: add pie support Liguzinski, WojciechX
2021-05-24 10:58 ` [dpdk-dev] [RFC PATCH 3/3] example/ip_pipeline: " Liguzinski, WojciechX
2021-05-24 16:19 ` [dpdk-dev] [RFC PATCH 0/3] Add PIE support for HQoS library Stephen Hemminger
2021-05-25  8:56 ` Morten Brørup
2021-06-07 13:01   ` Liguzinski, WojciechX
2021-06-09 10:53 ` [dpdk-dev] [RFC PATCH v1 " Liguzinski, WojciechX
2021-06-09 10:53   ` [dpdk-dev] [RFC PATCH v1 1/3] sched: add PIE based congestion management Liguzinski, WojciechX
2021-06-09 10:53   ` [dpdk-dev] [RFC PATCH v1 2/3] example/qos_sched: add PIE support Liguzinski, WojciechX
2021-06-09 10:53   ` [dpdk-dev] [RFC PATCH v1 3/3] example/ip_pipeline: " Liguzinski, WojciechX
2021-06-15  9:01   ` [dpdk-dev] [RFC PATCH v2 0/3] Add PIE support for HQoS library Liguzinski, WojciechX
2021-06-15  9:01     ` [dpdk-dev] [RFC PATCH v2 1/3] sched: add PIE based congestion management Liguzinski, WojciechX
2021-06-15  9:01     ` [dpdk-dev] [RFC PATCH v2 2/3] example/qos_sched: add PIE support Liguzinski, WojciechX
2021-06-15 12:23       ` Morten Brørup
2021-06-15  9:02     ` [dpdk-dev] [RFC PATCH v2 3/3] example/ip_pipeline: " Liguzinski, WojciechX
2021-06-21  7:35     ` [dpdk-dev] [RFC PATCH v3 0/3] Add PIE support for HQoS library Liguzinski, WojciechX
2021-06-21  7:35       ` [dpdk-dev] [RFC PATCH v3 1/3] sched: add PIE based congestion management Liguzinski, WojciechX
2021-06-21 18:17         ` Stephen Hemminger
2021-06-22  7:39           ` Liguzinski, WojciechX
2021-06-21  7:35       ` [dpdk-dev] [RFC PATCH v3 2/3] example/qos_sched: add PIE support Liguzinski, WojciechX
2021-06-21  7:35       ` [dpdk-dev] [RFC PATCH v3 3/3] example/ip_pipeline: " Liguzinski, WojciechX
2021-07-05  8:04       ` [dpdk-dev] [RFC PATCH v4 0/3] Add PIE support for HQoS library Liguzinski, WojciechX
2021-07-05  8:04         ` [dpdk-dev] [RFC PATCH v4 1/3] sched: add PIE based congestion management Liguzinski, WojciechX
2021-07-16 13:20           ` Dumitrescu, Cristian
2021-07-16 15:11           ` Dumitrescu, Cristian
2021-07-05  8:04         ` [dpdk-dev] [RFC PATCH v4 2/3] example/qos_sched: add pie support Liguzinski, WojciechX
2021-07-05  8:04         ` [dpdk-dev] [RFC PATCH v3 3/3] example/ip_pipeline: add PIE support Liguzinski, WojciechX
2021-07-16 12:46         ` [dpdk-dev] [RFC PATCH v4 0/3] Add PIE support for HQoS library Dumitrescu, Cristian

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MWHPR1101MB2112082D4CEB33C01604E6BF94369@MWHPR1101MB2112.namprd11.prod.outlook.com \
    --to=wojciechx.liguzinski@intel.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=jasvinder.singh@intel.com \
    --cc=mb@smartsharesystems.com \
    --cc=megha.ajmera@intel.com \
    --cc=savinay.dharmappa@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git