Hi lads, > -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Olivier MATZ > Sent: Monday, September 07, 2015 8:33 AM > To: Simon Kagstrom; dev at dpdk.org; Zhang, Helin; Gonzalez Monroy, Sergio; > Burakov, Anatoly > Subject: Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to > common code > > 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 <simon.kagstrom at netinsight.net> > > Signed-off-by: Johan Faltstrom <johan.faltstrom at netinsight.net> > > --- > > 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; > }
Wonder why do we need to do that? Probably head mbuf is out of space and want to expand it using pktmbuf_chain()? So in that case seems logical: 1) allocate new mbuf (it's pkt_len will be 0) b) call pktmbuf_chain() Konstantin > > > + /* 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
