[dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kagstrom]

Chaining/segmenting mbufs can be useful in many places, so make it
global.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Signed-off-by: Johan Faltstrom <johan.faltstrom@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
--- a/lib/librte_ip_frag/ip_frag_common.h
+++ b/lib/librte_ip_frag/ip_frag_common.h
@@ -166,27 +166,4 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
 	fp->frags[IP_FIRST_FRAG_IDX] = zero_frag;
 }
 
-/* chain two mbufs */
-static inline void
-ip_frag_chain(struct rte_mbuf *mn, struct rte_mbuf *mp)
-{
-	struct rte_mbuf *ms;
-
-	/* adjust start of the last fragment data. */
-	rte_pktmbuf_adj(mp, (uint16_t)(mp->l2_len + mp->l3_len));
-
-	/* chain two fragments. */
-	ms = rte_pktmbuf_lastseg(mn);
-	ms->next = mp;
-
-	/* accumulate number of segments and total length. */
-	mn->nb_segs = (uint8_t)(mn->nb_segs + mp->nb_segs);
-	mn->pkt_len += mp->pkt_len;
-
-	/* reset pkt_len and nb_segs for chained fragment. */
-	mp->pkt_len = mp->data_len;
-	mp->nb_segs = 1;
-}
-
-
 #endif /* _IP_FRAG_COMMON_H_ */
diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 5d24843..26d07f9 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -63,7 +63,9 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if(fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -78,7 +80,8 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 1f1c172..5969b4a 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -86,7 +86,9 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if (fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -101,7 +103,8 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
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;
+
+	/* 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);
+	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;
+}
+
+/**
  * Dump an mbuf structure to the console.
  *
  * Dump all fields for the given packet mbuf and all its associated
-- 
1.9.1

Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Olivier MATZ]

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@netinsight.net>
> Signed-off-by: Johan Faltstrom <johan.faltstrom@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;
}

> +	/* 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

Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Ananyev, Konstantin]

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 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@netinsight.net>
> > Signed-off-by: Johan Faltstrom <johan.faltstrom@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

Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Olivier MATZ]

Hi,

>>> 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()

By experience, having empty segments in the middle of a mbuf
chain is problematic (functions getting ptr at offsets, some pmds
or hardware may behave badly), I wanted to avoid that risk.

Now, the use-case you described is legitimate. Another option would
be to have another function pktmbuf_append_new(m) that returns a new
mbuf that is already chained to the other.

But I'm also fine with removing the test, it's maybe simpler.

Regards,
Olivier

Re: [dpdk-dev] [PATCH RFC] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kågström]

On 2015-09-07 11:35, Olivier MATZ wrote:

>> 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()
> 
> By experience, having empty segments in the middle of a mbuf
> chain is problematic (functions getting ptr at offsets, some pmds
> or hardware may behave badly), I wanted to avoid that risk.
> 
> Now, the use-case you described is legitimate. Another option would
> be to have another function pktmbuf_append_new(m) that returns a new
> mbuf that is already chained to the other.

I see with that method in that you have to remember to actually update
pkt_len in the head buffer when chaining an empty mbuf. Anyway, to
disallow this behavior should probably not be the responsibility of
rte_pktmbuf_chain(), so I'm fine with leaving the check out.

// Simon

[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kagstrom]

Chaining/segmenting mbufs can be useful in many places, so make it
global.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
---
ChangeLog:
v2:
  * Check for nb_segs byte overflow (Olivier MATZ)
  * Don't reset nb_segs in tail (Olivier MATZ)

 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               | 30 ++++++++++++++++++++++++++++++
 4 files changed, 40 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
--- a/lib/librte_ip_frag/ip_frag_common.h
+++ b/lib/librte_ip_frag/ip_frag_common.h
@@ -166,27 +166,4 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
 	fp->frags[IP_FIRST_FRAG_IDX] = zero_frag;
 }
 
-/* chain two mbufs */
-static inline void
-ip_frag_chain(struct rte_mbuf *mn, struct rte_mbuf *mp)
-{
-	struct rte_mbuf *ms;
-
-	/* adjust start of the last fragment data. */
-	rte_pktmbuf_adj(mp, (uint16_t)(mp->l2_len + mp->l3_len));
-
-	/* chain two fragments. */
-	ms = rte_pktmbuf_lastseg(mn);
-	ms->next = mp;
-
-	/* accumulate number of segments and total length. */
-	mn->nb_segs = (uint8_t)(mn->nb_segs + mp->nb_segs);
-	mn->pkt_len += mp->pkt_len;
-
-	/* reset pkt_len and nb_segs for chained fragment. */
-	mp->pkt_len = mp->data_len;
-	mp->nb_segs = 1;
-}
-
-
 #endif /* _IP_FRAG_COMMON_H_ */
diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 5d24843..26d07f9 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -63,7 +63,9 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if(fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -78,7 +80,8 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 1f1c172..5969b4a 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -86,7 +86,9 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if (fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -101,7 +103,8 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d7c9030..19a4bb5 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1775,6 +1775,36 @@ 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
+ *
+ * @return 0 on success, -EOVERFLOW if the chain is full (256 entries)
+ */
+static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
+{
+	struct rte_mbuf *cur_tail;
+
+	/* Check for number-of-segments-overflow */
+	if (head->nb_segs + tail->nb_segs >= sizeof(head->nb_segs) << 8)
+		return -EOVERFLOW;
+
+	/* 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);
+	head->pkt_len += tail->pkt_len;
+
+	/* pkt_len is only set in the head */
+	tail->pkt_len = tail->data_len;
+
+	return 0;
+}
+
+/**
  * Dump an mbuf structure to the console.
  *
  * Dump all fields for the given packet mbuf and all its associated
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Ananyev, Konstantin]

Hi Simon,
Looks good to me, just one nit, see below.
Konstantin 

>  /**
> + * 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
> + *
> + * @return 0 on success, -EOVERFLOW if the chain is full (256 entries)
> + */
> +static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
> +{
> +	struct rte_mbuf *cur_tail;
> +
> +	/* Check for number-of-segments-overflow */
> +	if (head->nb_segs + tail->nb_segs >= sizeof(head->nb_segs) << 8)
> +		return -EOVERFLOW;

Would probably be better 'sizeof(head->nb_segs) << CHAR_BIT', or even just: '  > UINT8_MAX'.
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);
> +	head->pkt_len += tail->pkt_len;
> +
> +	/* pkt_len is only set in the head */
> +	tail->pkt_len = tail->data_len;
> +
> +	return 0;
> +}
> +
> +/**
>   * Dump an mbuf structure to the console.
>   *
>   * Dump all fields for the given packet mbuf and all its associated
> --
> 1.9.1

Re: [dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kågström]

On 2015-09-07 14:32, Ananyev, Konstantin wrote:
>> +static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
>> +{
>> +	struct rte_mbuf *cur_tail;
>> +
>> +	/* Check for number-of-segments-overflow */
>> +	if (head->nb_segs + tail->nb_segs >= sizeof(head->nb_segs) << 8)
>> +		return -EOVERFLOW;
> 
> Would probably be better 'sizeof(head->nb_segs) << CHAR_BIT', or even just: '  > UINT8_MAX'.
> Konstantin

Thanks. I got it wrong anyway, what I wanted was to be able to handle
the day when nb_segs changes to a 16-bit number, but then it should
really be

  ... >= 1 << (sizeof(head->nb_segs) * 8)

anyway. I'll fix that and also add a warning that the implementation
will do a linear search to find the tail entry.

// Simon

Re: [dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Ananyev, Konstantin]

> -----Original Message-----
> From: Simon Kågström [mailto:simon.kagstrom@netinsight.net]
> Sent: Monday, September 07, 2015 1:41 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: Olivier MATZ; Zhang, Helin; Gonzalez Monroy, Sergio; Burakov, Anatoly
> Subject: Re: [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
> 
> On 2015-09-07 14:32, Ananyev, Konstantin wrote:
> >> +static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
> >> +{
> >> +	struct rte_mbuf *cur_tail;
> >> +
> >> +	/* Check for number-of-segments-overflow */
> >> +	if (head->nb_segs + tail->nb_segs >= sizeof(head->nb_segs) << 8)
> >> +		return -EOVERFLOW;
> >
> > Would probably be better 'sizeof(head->nb_segs) << CHAR_BIT', or even just: '  > UINT8_MAX'.
> > Konstantin
> 
> Thanks. I got it wrong anyway, what I wanted was to be able to handle
> the day when nb_segs changes to a 16-bit number, but then it should

Probably just me, but I can't foresee the situation when  we would need to increase nb_segs to 16 bits.
Looks like an overkill to me.
Konstantin


> really be
> 
>   ... >= 1 << (sizeof(head->nb_segs) * 8)
> 
> anyway. I'll fix that and also add a warning that the implementation
> will do a linear search to find the tail entry.
> 
> // Simon

Re: [dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kågström]

On 2015-09-08 01:21, Ananyev, Konstantin wrote:
>>
>> Thanks. I got it wrong anyway, what I wanted was to be able to handle
>> the day when nb_segs changes to a 16-bit number, but then it should
>> really be
>>
>>   ... >= 1 << (sizeof(head->nb_segs) * 8)
>>
>> anyway. I'll fix that and also add a warning that the implementation
>> will do a linear search to find the tail entry.
> 
> Probably just me, but I can't foresee the situation when  we would need to increase nb_segs to 16 bits.
> Looks like an overkill to me.

I don't think it will happen either, but with this solution, this
particular piece of code will work regardless. The value is known at
compile-time anyway, so it should not be a performance issue.

// Simon

Re: [dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

[Ananyev, Konstantin]

> -----Original Message-----
> From: Simon Kågström [mailto:simon.kagstrom@netinsight.net]
> Sent: Tuesday, September 08, 2015 11:41 AM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: Olivier MATZ; Zhang, Helin; Gonzalez Monroy, Sergio; Burakov, Anatoly
> Subject: Re: [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
> 
> On 2015-09-08 01:21, Ananyev, Konstantin wrote:
> >>
> >> Thanks. I got it wrong anyway, what I wanted was to be able to handle
> >> the day when nb_segs changes to a 16-bit number, but then it should
> >> really be
> >>
> >>   ... >= 1 << (sizeof(head->nb_segs) * 8)
> >>
> >> anyway. I'll fix that and also add a warning that the implementation
> >> will do a linear search to find the tail entry.
> >
> > Probably just me, but I can't foresee the situation when  we would need to increase nb_segs to 16 bits.
> > Looks like an overkill to me.
> 
> I don't think it will happen either, but with this solution, this
> particular piece of code will work regardless. The value is known at
> compile-time anyway, so it should not be a performance issue.

Ok :)
Konstantin

> 
> // Simon

[dpdk-dev] [PATCH v3] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kagstrom]

Chaining/segmenting mbufs can be useful in many places, so make it
global.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
---
ChangeLog:
v2:
  * Check for nb_segs byte overflow (Olivier MATZ)
  * Don't reset nb_segs in tail (Olivier MATZ)
v3:
  * Describe performance implications of linear search
  * Correct check-for-out-of-bounds (Konstantin Ananyev)

 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               | 34 ++++++++++++++++++++++++++++++++
 4 files changed, 44 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
--- a/lib/librte_ip_frag/ip_frag_common.h
+++ b/lib/librte_ip_frag/ip_frag_common.h
@@ -166,27 +166,4 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
 	fp->frags[IP_FIRST_FRAG_IDX] = zero_frag;
 }
 
-/* chain two mbufs */
-static inline void
-ip_frag_chain(struct rte_mbuf *mn, struct rte_mbuf *mp)
-{
-	struct rte_mbuf *ms;
-
-	/* adjust start of the last fragment data. */
-	rte_pktmbuf_adj(mp, (uint16_t)(mp->l2_len + mp->l3_len));
-
-	/* chain two fragments. */
-	ms = rte_pktmbuf_lastseg(mn);
-	ms->next = mp;
-
-	/* accumulate number of segments and total length. */
-	mn->nb_segs = (uint8_t)(mn->nb_segs + mp->nb_segs);
-	mn->pkt_len += mp->pkt_len;
-
-	/* reset pkt_len and nb_segs for chained fragment. */
-	mp->pkt_len = mp->data_len;
-	mp->nb_segs = 1;
-}
-
-
 #endif /* _IP_FRAG_COMMON_H_ */
diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
index 5d24843..26d07f9 100644
--- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
@@ -63,7 +63,9 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if(fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -78,7 +80,8 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
index 1f1c172..5969b4a 100644
--- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
+++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
@@ -86,7 +86,9 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 			/* previous fragment found. */
 			if (fp->frags[i].ofs + fp->frags[i].len == ofs) {
 
-				ip_frag_chain(fp->frags[i].mb, m);
+				/* adjust start of the last fragment data. */
+				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+				rte_pktmbuf_chain(fp->frags[i].mb, m);
 
 				/* update our last fragment and offset. */
 				m = fp->frags[i].mb;
@@ -101,7 +103,8 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
 	}
 
 	/* chain with the first fragment. */
-	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
+	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
+	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
 	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
 
 	/* update mbuf fields for reassembled packet. */
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d7c9030..f1f1400 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1775,6 +1775,40 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
 }
 
 /**
+ * Chain an mbuf to another, thereby creating a segmented packet.
+ *
+ * Note: The implementation will do a linear walk over the segments to find
+ * the tail entry. For cases when there are many segments, it's better to
+ * chain the entries manually.
+ *
+ * @param head the head of the mbuf chain (the first packet)
+ * @param tail the mbuf to put last in the chain
+ *
+ * @return 0 on success, -EOVERFLOW if the chain is full (256 entries)
+ */
+static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
+{
+	struct rte_mbuf *cur_tail;
+
+	/* Check for number-of-segments-overflow */
+	if (head->nb_segs + tail->nb_segs >= 1 << (sizeof(head->nb_segs) * 8))
+		return -EOVERFLOW;
+
+	/* 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);
+	head->pkt_len += tail->pkt_len;
+
+	/* pkt_len is only set in the head */
+	tail->pkt_len = tail->data_len;
+
+	return 0;
+}
+
+/**
  * Dump an mbuf structure to the console.
  *
  * Dump all fields for the given packet mbuf and all its associated
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] mbuf/ip_frag: Move mbuf chaining to common code

[Simon Kagstrom]

Ping?

// Simon

On Mon, 7 Sep 2015 14:50:09 +0200
Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> Chaining/segmenting mbufs can be useful in many places, so make it
> global.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
> ---
> ChangeLog:
> v2:
>   * Check for nb_segs byte overflow (Olivier MATZ)
>   * Don't reset nb_segs in tail (Olivier MATZ)
> v3:
>   * Describe performance implications of linear search
>   * Correct check-for-out-of-bounds (Konstantin Ananyev)
> 
>  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               | 34 ++++++++++++++++++++++++++++++++
>  4 files changed, 44 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
> --- a/lib/librte_ip_frag/ip_frag_common.h
> +++ b/lib/librte_ip_frag/ip_frag_common.h
> @@ -166,27 +166,4 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
>  	fp->frags[IP_FIRST_FRAG_IDX] = zero_frag;
>  }
>  
> -/* chain two mbufs */
> -static inline void
> -ip_frag_chain(struct rte_mbuf *mn, struct rte_mbuf *mp)
> -{
> -	struct rte_mbuf *ms;
> -
> -	/* adjust start of the last fragment data. */
> -	rte_pktmbuf_adj(mp, (uint16_t)(mp->l2_len + mp->l3_len));
> -
> -	/* chain two fragments. */
> -	ms = rte_pktmbuf_lastseg(mn);
> -	ms->next = mp;
> -
> -	/* accumulate number of segments and total length. */
> -	mn->nb_segs = (uint8_t)(mn->nb_segs + mp->nb_segs);
> -	mn->pkt_len += mp->pkt_len;
> -
> -	/* reset pkt_len and nb_segs for chained fragment. */
> -	mp->pkt_len = mp->data_len;
> -	mp->nb_segs = 1;
> -}
> -
> -
>  #endif /* _IP_FRAG_COMMON_H_ */
> diff --git a/lib/librte_ip_frag/rte_ipv4_reassembly.c b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> index 5d24843..26d07f9 100644
> --- a/lib/librte_ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv4_reassembly.c
> @@ -63,7 +63,9 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
>  			/* previous fragment found. */
>  			if(fp->frags[i].ofs + fp->frags[i].len == ofs) {
>  
> -				ip_frag_chain(fp->frags[i].mb, m);
> +				/* adjust start of the last fragment data. */
> +				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
> +				rte_pktmbuf_chain(fp->frags[i].mb, m);
>  
>  				/* update our last fragment and offset. */
>  				m = fp->frags[i].mb;
> @@ -78,7 +80,8 @@ ipv4_frag_reassemble(const struct ip_frag_pkt *fp)
>  	}
>  
>  	/* chain with the first fragment. */
> -	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
> +	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
>  
>  	/* update mbuf fields for reassembled packet. */
> diff --git a/lib/librte_ip_frag/rte_ipv6_reassembly.c b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> index 1f1c172..5969b4a 100644
> --- a/lib/librte_ip_frag/rte_ipv6_reassembly.c
> +++ b/lib/librte_ip_frag/rte_ipv6_reassembly.c
> @@ -86,7 +86,9 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
>  			/* previous fragment found. */
>  			if (fp->frags[i].ofs + fp->frags[i].len == ofs) {
>  
> -				ip_frag_chain(fp->frags[i].mb, m);
> +				/* adjust start of the last fragment data. */
> +				rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
> +				rte_pktmbuf_chain(fp->frags[i].mb, m);
>  
>  				/* update our last fragment and offset. */
>  				m = fp->frags[i].mb;
> @@ -101,7 +103,8 @@ ipv6_frag_reassemble(const struct ip_frag_pkt *fp)
>  	}
>  
>  	/* chain with the first fragment. */
> -	ip_frag_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
> +	rte_pktmbuf_adj(m, (uint16_t)(m->l2_len + m->l3_len));
> +	rte_pktmbuf_chain(fp->frags[IP_FIRST_FRAG_IDX].mb, m);
>  	m = fp->frags[IP_FIRST_FRAG_IDX].mb;
>  
>  	/* update mbuf fields for reassembled packet. */
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index d7c9030..f1f1400 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1775,6 +1775,40 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
>  }
>  
>  /**
> + * Chain an mbuf to another, thereby creating a segmented packet.
> + *
> + * Note: The implementation will do a linear walk over the segments to find
> + * the tail entry. For cases when there are many segments, it's better to
> + * chain the entries manually.
> + *
> + * @param head the head of the mbuf chain (the first packet)
> + * @param tail the mbuf to put last in the chain
> + *
> + * @return 0 on success, -EOVERFLOW if the chain is full (256 entries)
> + */
> +static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail)
> +{
> +	struct rte_mbuf *cur_tail;
> +
> +	/* Check for number-of-segments-overflow */
> +	if (head->nb_segs + tail->nb_segs >= 1 << (sizeof(head->nb_segs) * 8))
> +		return -EOVERFLOW;
> +
> +	/* 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);
> +	head->pkt_len += tail->pkt_len;
> +
> +	/* pkt_len is only set in the head */
> +	tail->pkt_len = tail->data_len;
> +
> +	return 0;
> +}
> +
> +/**
>   * Dump an mbuf structure to the console.
>   *
>   * Dump all fields for the given packet mbuf and all its associated

Re: [dpdk-dev] [PATCH v3] mbuf/ip_frag: Move mbuf chaining to common code

[Thomas Monjalon]

2015-10-13 14:50, Simon Kagstrom:
> Ping?

OK you apply the ping method we have just talked about :)
To make it really effective, you should have these headers:
	To: Olivier Matz
	Cc: dev@dpdk.org
Indeed, as the mbuf maintainer, he's the target of your ping.

And to make it clear, the title should be
	mbuf: move chaining from ip_frag library
(note the lowercase in "move")

Now you know how to do it so you can spread the word when someone forget
these advices.
Thanks

Re: [dpdk-dev] [PATCH v3] mbuf/ip_frag: Move mbuf chaining to common code

[Olivier MATZ]

Hi Simon,

On 09/07/2015 02:50 PM, Simon Kagstrom wrote:
> Chaining/segmenting mbufs can be useful in many places, so make it
> global.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
> 
> [...]
> 
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1775,6 +1775,40 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
>  }
>  
>  /**
> + * Chain an mbuf to another, thereby creating a segmented packet.
> + *
> + * Note: The implementation will do a linear walk over the segments to find
> + * the tail entry. For cases when there are many segments, it's better to
> + * chain the entries manually.
> + *
> + * @param head the head of the mbuf chain (the first packet)
> + * @param tail the mbuf to put last in the chain
> + *
> + * @return 0 on success, -EOVERFLOW if the chain is full (256 entries)
> + */

Small nit about the API comment, it should be:

@param head
  The head of the mbuf chain (the first packet).
...

(note the uppercase and the dot at the end, see the other functions
in the file)

I know Thomas usually fixes this kind of stuff when he pushes
the patches, but it's better if we can avoid him this load :)


Regards,
Olivier