From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4099471B3 for ; Thu, 11 Jan 2018 17:42:32 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2018 08:42:30 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,345,1511856000"; d="scan'208";a="9017657" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga008.fm.intel.com with ESMTP; 11 Jan 2018 08:42:29 -0800 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.167]) by IRSMSX106.ger.corp.intel.com ([169.254.8.36]) with mapi id 14.03.0319.002; Thu, 11 Jan 2018 16:42:28 +0000 From: "Dumitrescu, Cristian" To: "alanrobertsonatt@gmail.com" CC: "dev@dpdk.org" , Alan Robertson , "Alan Robertson" Thread-Topic: [PATCH] Allow -ve frame_overhead values Thread-Index: AQHTeAHPRkbs3OHA5EeWUAQU4NhMsaNvAd2Q Date: Thu, 11 Jan 2018 16:42:27 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BAFBDEF@IRSMSX108.ger.corp.intel.com> References: <1513602715-25078-1-git-send-email-alanrobertsatt@gmail.com> In-Reply-To: <1513602715-25078-1-git-send-email-alanrobertsatt@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] Allow -ve frame_overhead values X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jan 2018 16:42:32 -0000 Hi Alan, I agree, having a signed framing_overhead makes sense for several cases, in= cluding the TDM case you mention. In fact, this is the approach used by rte= _tm.h. Several issues to fix below. > -----Original Message----- > From: alanrobertsonatt@gmail.com [mailto:alanrobertsonatt@gmail.com] > Sent: Monday, December 18, 2017 1:12 PM > To: Dumitrescu, Cristian > Cc: dev@dpdk.org; Alan Robertson ; Alan Robertson > > Subject: [PATCH] Allow -ve frame_overhead values >=20 > From: Alan Robertson >=20 > When forwarding traffic across a TDM network the ethernet header will > be replaced with a ML-PPP one thereby reducing the size of the packet. >=20 > Signed-off-by: Alan Robertson > --- > lib/librte_sched/rte_sched.c | 41 > ++++++++++++++++++++++++++++++++++++----- > lib/librte_sched/rte_sched.h | 2 +- > 2 files changed, 37 insertions(+), 6 deletions(-) >=20 > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index 7252f850d..5c88f1b62 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -216,7 +216,7 @@ struct rte_sched_port { > uint32_t n_pipes_per_subport; > uint32_t rate; > uint32_t mtu; > - uint32_t frame_overhead; > + int32_t frame_overhead; > uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; > uint32_t n_pipe_profiles; > uint32_t pipe_tc3_rate_max; > @@ -643,7 +643,14 @@ rte_sched_port_config(struct > rte_sched_port_params *params) > port->n_subports_per_port =3D params->n_subports_per_port; > port->n_pipes_per_subport =3D params->n_pipes_per_subport; > port->rate =3D params->rate; > - port->mtu =3D params->mtu + params->frame_overhead; > + > + /* Only add a +ve overhead. A -ve overhead is to accommodate > + * for downstream TDM devices which won't have an ethernet > header, > + * they obviously won't impact our local interface MTU size. > + */ No need for this comment here. Also, we don't want to mention any specific = protocols unless we want to create a cvasi-exhaustive list of protocols, wh= ich breaks the design guideline of having protocol agnostic scheduling libr= ary. > + port->mtu =3D params->mtu; > + if (params->frame_overhead > 0) > + port->mtu +=3D params->frame_overhead; This is not really correct, the MTU should always be: port->mtu =3D params->mtu + params->frame_overhead; > port->frame_overhead =3D params->frame_overhead; > memcpy(port->qsize, params->qsize, sizeof(params->qsize)); > port->n_pipe_profiles =3D params->n_pipe_profiles; > @@ -1613,13 +1620,21 @@ grinder_credits_check(struct rte_sched_port > *port, uint32_t pos) > struct rte_sched_pipe *pipe =3D grinder->pipe; > struct rte_mbuf *pkt =3D grinder->pkt; > uint32_t tc_index =3D grinder->tc_index; > - uint32_t pkt_len =3D pkt->pkt_len + port->frame_overhead; > + uint32_t pkt_len; > + int32_t tpkt_len; The variable tpkt_len is not really needed. > uint32_t subport_tb_credits =3D subport->tb_credits; > uint32_t subport_tc_credits =3D subport->tc_credits[tc_index]; > uint32_t pipe_tb_credits =3D pipe->tb_credits; > uint32_t pipe_tc_credits =3D pipe->tc_credits[tc_index]; > int enough_credits; >=20 > + /* Make sure we don't allow this to go -ve. To accommodate > + * downstream TDM devices we may want to ignore the ethernet > + * header so allow -ve overhead values. > + */ As stated above, no need for this comment. > + tpkt_len =3D pkt->pkt_len + port->frame_overhead; > + pkt_len =3D (tpkt_len < 0) ? 1 : tpkt_len; > + It has to be application responsibility to make sure this case does not hap= pen, i.e. each input packet always contains the header(s) covered by the fr= aming overhead value which are to be removed at some later point. IMO this code is not correct, as it is more a cover-up than a fix, therefor= e it should be removed, so we always have: uint32_t pkt_len =3D pkt->pkt_len + port->frame_overhead; To protect from misconfigurations, please replace this code with a debug as= sert/check under #ifdef RTE_SCHED_DEBUG. > /* Check queue credits */ > enough_credits =3D (pkt_len <=3D subport_tb_credits) && > (pkt_len <=3D subport_tc_credits) && > @@ -1648,7 +1663,8 @@ grinder_credits_check(struct rte_sched_port > *port, uint32_t pos) > struct rte_sched_pipe *pipe =3D grinder->pipe; > struct rte_mbuf *pkt =3D grinder->pkt; > uint32_t tc_index =3D grinder->tc_index; > - uint32_t pkt_len =3D pkt->pkt_len + port->frame_overhead; > + uint32_t pkt_len; > + int32_t tpkt_len; > uint32_t subport_tb_credits =3D subport->tb_credits; > uint32_t subport_tc_credits =3D subport->tc_credits[tc_index]; > uint32_t pipe_tb_credits =3D pipe->tb_credits; > @@ -1658,6 +1674,13 @@ grinder_credits_check(struct rte_sched_port > *port, uint32_t pos) > uint32_t pipe_tc_ov_credits =3D pipe_tc_ov_mask1[tc_index]; > int enough_credits; >=20 > + /* Make sure we don't allow this to go -ve. To accommodate > + * downstream TDM devices we may want to ignore the ethernet > + * header so allow -ve overhead values. > + */ > + tpkt_len =3D pkt->pkt_len + port->frame_overhead; > + pkt_len =3D (tpkt_len < 0) ? 1 : tpkt_len; > + Same comments from above apply here as well. > /* Check pipe and subport credits */ > enough_credits =3D (pkt_len <=3D subport_tb_credits) && > (pkt_len <=3D subport_tc_credits) && > @@ -1687,11 +1710,19 @@ grinder_schedule(struct rte_sched_port *port, > uint32_t pos) > struct rte_sched_grinder *grinder =3D port->grinder + pos; > struct rte_sched_queue *queue =3D grinder->queue[grinder->qpos]; > struct rte_mbuf *pkt =3D grinder->pkt; > - uint32_t pkt_len =3D pkt->pkt_len + port->frame_overhead; > + uint32_t pkt_len; > + int32_t tpkt_len; >=20 > if (!grinder_credits_check(port, pos)) > return 0; >=20 > + /* Make sure we don't allow this to go -ve. To accommodate > + * downstream TDM devices we may want to ignore the ethernet > + * header so allow -ve overhead values. > + */ > + tpkt_len =3D pkt->pkt_len + port->frame_overhead; > + pkt_len =3D (tpkt_len < 0) ? 1 : tpkt_len; > + Same here. > /* Advance port time */ > port->time +=3D pkt_len; >=20 > diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h > index e9c281726..63677eefc 100644 > --- a/lib/librte_sched/rte_sched.h > +++ b/lib/librte_sched/rte_sched.h > @@ -219,7 +219,7 @@ struct rte_sched_port_params { > uint32_t mtu; /**< Maximum Ethernet frame size > * (measured in bytes). > * Should not include the framing > overhead. */ > - uint32_t frame_overhead; /**< Framing overhead per packet > + int32_t frame_overhead; /**< Framing overhead per packet > * (measured in bytes) */ I would augment the comment with: /**< Framing overhead per packet (measured in bytes). Can have negative va= lue. */ > uint32_t n_subports_per_port; /**< Number of subports */ > uint32_t n_pipes_per_subport; /**< Number of pipes per subport > */ > -- > 2.11.0 Regards, Cristian