From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id CCCA35A1F for ; Mon, 7 Sep 2015 09:32:51 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1ZYqv9-0001Cb-EZ; Mon, 07 Sep 2015 09:32:56 +0200 Message-ID: <55ED3D9A.7070607@6wind.com> Date: Mon, 07 Sep 2015 09:32:42 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Simon Kagstrom , dev@dpdk.org, helin.zhang@intel.com, sergio.gonzalez.monroy@intel.com, anatoly.burakov@intel.com References: <20150831144110.4a7afa27@miho> In-Reply-To: <20150831144110.4a7afa27@miho> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 07:32:52 -0000 Hi Simon, I think it's a good idea. Please see some minor comments below. 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/ip_frag_common.h > index 6b2acee..cde6ed4 100644 > [...] > 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(const 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; > + 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. if (unlikely(rte_pktmbuf_pkt_len(tail) == 0)) { rte_pktmbuf_free(tail); return; } > + /* Chain 'tail' onto the old tail */ > + cur_tail = rte_pktmbuf_lastseg(head); > + cur_tail->next = tail; > + > + /* accumulate number of segments and total length. */ > + head->nb_segs = (uint8_t)(head->nb_segs + tail->nb_segs); 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. > + head->pkt_len += tail->pkt_len; > + > + /* reset pkt_len and nb_segs for chained fragment. */ > + tail->pkt_len = tail->data_len; > + tail->nb_segs = 1; I don't think it's required to reset this fields in the tail mbuf. In any case, they will be reset again. > +} > + > +/** > * Dump an mbuf structure to the console. > * > * Dump all fields for the given packet mbuf and all its associated > Regards, Olivier