From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6BBA1A0546; Tue, 25 May 2021 11:16:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 545CB40150; Tue, 25 May 2021 11:16:46 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 5EAB84003F for ; Tue, 25 May 2021 11:16:45 +0200 (CEST) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Tue, 25 May 2021 11:16:43 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C617C3@smartserver.smartshare.dk> In-Reply-To: <20210524105822.63171-2-wojciechx.liguzinski@intel.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management Thread-Index: AddQi9opR3bgbN9CRQC2HAghrX99JQAuBS3A References: <20210524105822.63171-1-wojciechx.liguzinski@intel.com> <20210524105822.63171-2-wojciechx.liguzinski@intel.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Liguzinski, WojciechX" , , , Cc: Subject: Re: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liguzinski, > WojciechX > Sent: Monday, 24 May 2021 12.58 >=20 > Implement pie based congestion management based on rfc8033 >=20 > Signed-off-by: Liguzinski, WojciechX > --- > 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 =3D 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) { > + > + uint32_t j; > + > + for (j =3D 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) =3D=3D 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) !=3D 0) { > + rte_sched_free_memory(port, n_subports); > + > + RTE_LOG(NOTICE, SCHED, > + "%s: RED configuration init fails\n", > __func__); > + return -EINVAL; > + } > + } > + } > + s->cman =3D 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 =3D 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) !=3D 0) { > + rte_sched_free_memory(port, n_subports); > + > + RTE_LOG(NOTICE, SCHED, > + "%s: PIE configuration init fails\n", __func__); > + return -EINVAL; > + } > + } > + s->cman =3D 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 =3D subport->queue_extra + qindex; > + struct rte_pie *pie =3D &qe->pie; > + > + /* Update queue length */ > + pie->qlen -=3D 1; > + pie->qlen_bytes -=3D 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 >=20 > +/** > + * 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 >=20 >=20 > 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. >=20 Please don't use this footer when sending to the DPDK mailing list.