From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 631045693 for ; Mon, 7 Sep 2015 11:13:26 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 07 Sep 2015 02:13:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,484,1437462000"; d="scan'208";a="799329576" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga002.jf.intel.com with ESMTP; 07 Sep 2015 02:13:25 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX151.ger.corp.intel.com ([169.254.4.37]) with mapi id 14.03.0224.002; Mon, 7 Sep 2015 10:13:22 +0100 From: "Ananyev, Konstantin" To: Olivier MATZ , Simon Kagstrom , "dev@dpdk.org" , "Zhang, Helin" , "Gonzalez Monroy, Sergio" , "Burakov, Anatoly" Thread-Topic: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code Thread-Index: AQHQ6T+IukEPSb/dR06GyXVGZqVeF54wxv8Q Date: Mon, 7 Sep 2015 09:13:21 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836A83CBA@irsmsx105.ger.corp.intel.com> References: <20150831144110.4a7afa27@miho> <55ED3D9A.7070607@6wind.com> In-Reply-To: <55ED3D9A.7070607@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 RFC] mbuf/ip_frag: Move mbuf chaining to common code X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Sep 2015 09:13:27 -0000 Hi lads, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ > Sent: Monday, September 07, 2015 8:33 AM > To: Simon Kagstrom; dev@dpdk.org; Zhang, Helin; Gonzalez Monroy, Sergio; = Burakov, Anatoly > Subject: Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to c= ommon code >=20 > Hi Simon, >=20 > I think it's a good idea. Please see some minor comments below. >=20 > On 08/31/2015 02:41 PM, Simon Kagstrom wrote: > > Chaining/segmenting mbufs can be useful in many places, so make it > > global. > > > > Signed-off-by: Simon Kagstrom > > Signed-off-by: Johan Faltstrom > > --- > > NOTE! Only compile-tested. > > > > We were looking for packet segmenting functionality in the MBUF API but > > didn't find it. This patch moves the implementation, apart from the > > things which look ip_frag-specific. > > > > lib/librte_ip_frag/ip_frag_common.h | 23 ----------------------- > > lib/librte_ip_frag/rte_ipv4_reassembly.c | 7 +++++-- > > lib/librte_ip_frag/rte_ipv6_reassembly.c | 7 +++++-- > > lib/librte_mbuf/rte_mbuf.h | 23 +++++++++++++++++++++++ > > 4 files changed, 33 insertions(+), 27 deletions(-) > > > > diff --git a/lib/librte_ip_frag/ip_frag_common.h b/lib/librte_ip_frag/i= p_frag_common.h > > index 6b2acee..cde6ed4 100644 >=20 > > [...] >=20 > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index 8c2db1b..ef47256 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -1801,6 +1801,29 @@ static inline int rte_pktmbuf_is_contiguous(cons= t struct rte_mbuf *m) > > } > > > > /** > > + * Chain an mbuf to another, thereby creating a segmented packet. > > + * > > + * @param head the head of the mbuf chain (the first packet) > > + * @param tail the mbuf to put last in the chain > > + */ > > +static inline void rte_pktmbuf_chain(struct rte_mbuf *head, struct rte= _mbuf *tail) > > +{ > > + struct rte_mbuf *cur_tail; > > + >=20 > Here, we could check if the pkt_len of tail mbuf is 0. If > it's the case, we can just free it and return. It would avoid > to have an empty segment inside the mbuf chain, which can be > annoying. >=20 > if (unlikely(rte_pktmbuf_pkt_len(tail) =3D=3D 0)) { > rte_pktmbuf_free(tail); > return; > } Wonder why do we need to do that? Probably head mbuf is out of space and want to expand it using pktmbuf_chai= n()? So in that case seems logical: 1) allocate new mbuf (it's pkt_len will be 0) b) call pktmbuf_chain() Konstantin >=20 > > + /* Chain 'tail' onto the old tail */ > > + cur_tail =3D rte_pktmbuf_lastseg(head); > > + cur_tail->next =3D tail; > > + > > + /* accumulate number of segments and total length. */ > > + head->nb_segs =3D (uint8_t)(head->nb_segs + tail->nb_segs); >=20 > I'm wondering if we shouldn't check the overflow here. In > this case we would need to have a return value in case of > failure. >=20 > > + head->pkt_len +=3D tail->pkt_len; > > + > > + /* reset pkt_len and nb_segs for chained fragment. */ > > + tail->pkt_len =3D tail->data_len; > > + tail->nb_segs =3D 1; >=20 > I don't think it's required to reset this fields in the tail mbuf. > In any case, they will be reset again. >=20 > > +} > > + > > +/** > > * Dump an mbuf structure to the console. > > * > > * Dump all fields for the given packet mbuf and all its associated > > >=20 >=20 > Regards, > Olivier