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 78F90A0C46; Wed, 9 Jun 2021 14:35:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 56F5F410F0; Wed, 9 Jun 2021 14:35:10 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 0BE8A4003C for ; Wed, 9 Jun 2021 14:35:09 +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: Wed, 9 Jun 2021 14:35:06 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C61838@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [dpdk-dev] [RFC PATCH 1/3] sched: add pie based congestion management Thread-Index: AQHXUIvyj+E+DTWn10COP3hFZZoy3qrz7EKAgBXaWYCAAeoocA== References: <20210524105822.63171-1-wojciechx.liguzinski@intel.com> <20210524105822.63171-2-wojciechx.liguzinski@intel.com> <98CBD80474FA8B44BF855DF32C47DC35C617C3@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Liguzinski, WojciechX" , "Singh, Jasvinder" , "Dumitrescu, Cristian" Cc: "Dharmappa, Savinay" , , "Ajmera, Megha" 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: Wednesday, 9 June 2021 10.37 >=20 > > From: Morten Br=F8rup > > Sent: Tuesday, May 25, 2021 11:17 AM > > > > > 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 > > > > --- > > > 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. >=20 > Ok, sure, I'm going to change that where applicable. >=20 > > > > > +#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. >=20 > 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'. >=20 Then it makes sense keeping them here. You can ignore my suggestion. > > > > > -#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. >=20 > Thanks, it's going to be fixed. >=20 > > 2. I suggest using "drops" as the variable name instead of "red" or > "aqm". >=20 > Ok, I will change that. >=20 > > > > > -#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. >=20 > Ok, it's going to be changed. >=20 > > > > > +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. >=20 > 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. Now that I understand your line of thinking, I agree with you. You can = ignore my comment here too. >=20 > > > > > diff --git a/lib/sched/rte_sched.h b/lib/sched/rte_sched.h > > > > > > +/** > > > + * Congestion management (CMAN) mode > > > > "Active Queue Management (AQM) mode", please. >=20 > Sure. ;-) >=20 > > > > > + * > > > + * 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. >=20 > 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. >=20 I don't have a strong opinion about this, and you are putting some = thoughts into it, so I'm happy with that. > > > > > + RTE_SCHED_CMAN_PIE, /**< Proportional Integral Controller > > > Enhanced (PIE) */ > > > +}; > > > + > > [snip] > Footer issue has been handled. >=20 > Thanks, > Wojtek :-)