[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
> -Original Message- > From: Simon K?gstr?m [mailto:simon.kagstrom at netinsight.net] > Sent: Tuesday, September 08, 2015 11:41 AM > To: Ananyev, Konstantin; dev at 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 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. // Simon
[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
> -Original Message-
> From: Simon K?gstr?m [mailto:simon.kagstrom at netinsight.net]
> Sent: Monday, September 07, 2015 1:41 PM
> To: Ananyev, Konstantin; dev at 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
[dpdk-dev] [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
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
[dpdk-dev] [PATCH v2] 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
Signed-off-by: Johan Faltstrom
---
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_pkt
[dpdk-dev] [PATCH v2] mbuf/ip_frag: Move mbuf chaining to common code
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
