mbuf/ip_frag: Move mbuf chaining to common code

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

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

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

lib/librte_ip_frag/ip_frag_common.h +0 -0

index 6b2acee..cde6ed4 100644
 
[...]

lib/librte_mbuf/rte_mbuf.h +0 -0

index 8c2db1b..ef47256 100644
@@ -1801,6 +1801,29 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
 }
 

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Olivier MATZ
Sent: Monday, September 07, 2015 8:33 AM
To: Simon Kagstrom; [email protected]; 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.

Quoted 30 lines · Simon Kagstrom

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

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

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

lib/librte_ip_frag/ip_frag_common.h +0 -0

index 6b2acee..cde6ed4 100644
 
[...]

lib/librte_mbuf/rte_mbuf.h +0 -0

index 8c2db1b..ef47256 100644
@@ -1801,6 +1801,29 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
 }
 

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1812

Quoted 10 lines

lib/librte_mbuf/rte_mbuf.h +9 -0

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

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1818

Quoted 7 lines

lib/librte_mbuf/rte_mbuf.h +6 -0

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

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1823

Quoted 5 lines

lib/librte_mbuf/rte_mbuf.h +5 -0

+	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.

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1829

Quoted 7 lines

lib/librte_mbuf/rte_mbuf.h +3 -0

+}
+
+/**
  * Dump an mbuf structure to the console.
  *
  * Dump all fields for the given packet mbuf and all its associated

Regards,
Olivier

Quoted 23 lines

Quoted 7 lines

lib/librte_mbuf/rte_mbuf.h +0 -0

index 8c2db1b..ef47256 100644
@@ -1801,6 +1801,29 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
 }
 

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1812

Quoted 10 lines

lib/librte_mbuf/rte_mbuf.h +9 -0

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

Quoted 4 lines

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

Quoted 5 lines

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.

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

lib/librte_mbuf/rte_mbuf.h +16 -1

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

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1791

Quoted 7 lines

lib/librte_mbuf/rte_mbuf.h +7 -0

+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

-----Original Message-----
From: Simon Kågström [mailto:[email protected]]
Sent: Monday, September 07, 2015 1:41 PM
To: Ananyev, Konstantin; [email protected]
Cc: Olivier MATZ; Zhang, Helin; Gonzalez Monroy, Sergio; Burakov, Anatoly
Subject: Re: [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

Quoted 10 lines · Konstantin

lib/librte_mbuf/rte_mbuf.h static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m) · line 1791

Quoted 7 lines

lib/librte_mbuf/rte_mbuf.h +7 -0

+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

Quoted 5 lines

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

Quoted 4 lines

... >= 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.

-----Original Message-----
From: Simon Kågström [mailto:[email protected]]
Sent: Tuesday, September 08, 2015 11:41 AM
To: Ananyev, Konstantin; [email protected]
Cc: Olivier MATZ; Zhang, Helin; Gonzalez Monroy, Sergio; Burakov, Anatoly
Subject: Re: [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code

Quoted 10 lines · Konstantin

Quoted 9 lines

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.

Quoted 2 lines

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

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

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

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

lib/librte_ip_frag/ip_frag_common.h +0 -23

index 6b2acee..cde6ed4 100644
@@ -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_ */

lib/librte_ip_frag/rte_ipv4_reassembly.c +5 -2

index 5d24843..26d07f9 100644
@@ -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. */

lib/librte_ip_frag/rte_ipv6_reassembly.c +5 -2

index 1f1c172..5969b4a 100644
@@ -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. */

lib/librte_mbuf/rte_mbuf.h +34 -0

index d7c9030..f1f1400 100644
@@ -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

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

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

[...]

lib/librte_mbuf/rte_mbuf.h +1 -0

@@ -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.

Ping?