[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Pattan, Reshma]

Hi,

> > >-Original Message-
> > >From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen
> > >Hemminger
> > >Sent: Friday, July 10, 2015 1:38 AM
> > >To: dev at dpdk.org
> > >Cc: Mike Davison ; Stephen Hemminger
> > >
> > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > >
> > >From: Stephen Hemminger 
> > >
> > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > >is common issue and can be problematic.
> > >
> > >Signed-off-by: Mike Davison 
> > >Reviewed-by: Stephen Hemminger 
> > >---
> >
> > Hi Stephen :>
> > This patch look useful in case of backup buffs.
> > There will be second approach ?
> 
> I did bother resubmitting it since unless there are users in current code 
> base it
> would likely rot.

I was writing similar mbuf copy functionality  which is required for tcpdump 
feature.
But, It was brought to my notice that this patch with similar functionality 
already  exists, so I would like to take this patch and work on further.
Also, if you have any further code changes on this patch, would you please send 
 the latest one. I will work further.

Thanks,
Reshma

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Fri, 18 Mar 2016 17:03:51 +
"Pattan, Reshma"  wrote:

> Hi,
> 
> > > >-Original Message-
> > > >From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen
> > > >Hemminger
> > > >Sent: Friday, July 10, 2015 1:38 AM
> > > >To: dev at dpdk.org
> > > >Cc: Mike Davison ; Stephen Hemminger
> > > >
> > > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > > >
> > > >From: Stephen Hemminger 
> > > >
> > > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > > >is common issue and can be problematic.
> > > >
> > > >Signed-off-by: Mike Davison 
> > > >Reviewed-by: Stephen Hemminger 
> > > >---
> > >
> > > Hi Stephen :>
> > > This patch look useful in case of backup buffs.
> > > There will be second approach ?
> > 
> > I did bother resubmitting it since unless there are users in current code 
> > base it
> > would likely rot.
> 
> I was writing similar mbuf copy functionality  which is required for tcpdump 
> feature.
> But, It was brought to my notice that this patch with similar functionality 
> already  exists, so I would like to take this patch and work on further.
> Also, if you have any further code changes on this patch, would you please 
> send  the latest one. I will work further.
> 
> Thanks,
> Reshma

We have a newer version that handles different size pools.

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Wed, 3 Feb 2016 17:23:05 +
Olivier MATZ  wrote:

>  +} while ((md = md->next) != NULL);
> >> +
> >> +  *prev = NULL;  
> 
> why this?
This is null terminating the linked list of segments.

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Olivier MATZ]

Hi Stephen,

Please find some comments below.

On 01/22/2016 02:40 PM, Mrzyglod, DanielX T wrote:
>> +/*
>> + * Creates a copy of the given packet mbuf.
>> + *
>> + * Walks through all segments of the given packet mbuf, and for each of 
>> them:
>> + *  - Creates a new packet mbuf from the given pool.
>> + *  - Copy segment to newly created mbuf.
>> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values
>> + * from the original packet mbuf.
>> + *
>> + * @param md
>> + *   The packet mbuf to be copied.
>> + * @param mp
>> + *   The mempool from which the mbufs are allocated.
>> + * @return
>> + *   - The pointer to the new mbuf on success.
>> + *   - NULL if allocation fails.
>> + */
>> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
>> +struct rte_mempool *mp)

You are usually against inline functions. Any reason why it's better
to inline the code in this case?

Also, I think the mbuf argument should be const.

>> +{
>> +struct rte_mbuf *mc = NULL;
>> +struct rte_mbuf **prev = 
>> +
>> +do {
>> +struct rte_mbuf *mi;
>> +
>> +mi = rte_pktmbuf_alloc(mp);
>> +if (unlikely(mi == NULL)) {
>> +rte_pktmbuf_free(mc);
>> +return NULL;
>> +}
>> +
>> +mi->data_off = md->data_off;

I'm not a big fan of 'mi' and 'md' (and 'mc'). In rte_pktmbuf_attach(),
'md' means direct and 'mi' means indirect. Here, all mbufs are direct
(i.e. they points to their own data buffer).

I think that using 'm' and 'm2' (or m_dup) is clearer. Also, 'seg' looks
clearer for a mbuf that points to a rte_mbuf inside the chain.

>> +mi->data_len = md->data_len;
>> +mi->port = md->port;

We don't need to update port for all the segments here.
Same for vlan_tci, tx_offload, hash, pkt_len, nb_segs, ol_flags,
packet_type.

>> +mi->vlan_tci = md->vlan_tci;
>> +mi->tx_offload = md->tx_offload;
>> +mi->hash = md->hash;
>> +
>> +mi->next = NULL;
>> +mi->pkt_len = md->pkt_len;
>> +mi->nb_segs = md->nb_segs;
>> +mi->ol_flags = md->ol_flags;
>> +mi->packet_type = md->packet_type;
>> +
>> +rte_memcpy(rte_pktmbuf_mtod(mi, char *),
>> +   rte_pktmbuf_mtod(md, char *),
>> +   md->data_len);
>> +
>> +*prev = mi;
>> +prev = >next;

Using a double mbuf pointer here looks a bit overkill to me.
I suggest to do something like (just an example, not even compiled):

struct rte_mbuf *rte_pktmbuf_copy(const struct rte_mbuf *m,
struct rte_mempool *mp)
{
struct rte_mbuf *m2, *seg, *seg2;

m2 = rte_pktmbuf_alloc(mp);
if (unlikely(m2 == NULL)) {
rte_pktmbuf_free(m2);
return NULL;
}
m2->data_off = m->data_off;
m2->data_len = m->data_len;
m2->pkt_len = m->pkt_len;
m2->tx_offload = m->tx_offload;
/* ... */

for (seg = m->next; seg != NULL; seg = seg->next) {

seg2 = rte_pktmbuf_alloc(mp);
if (unlikely(seg2 == NULL)) {
rte_pktmbuf_free(m2);
return NULL;
}

seg2->data_off = seg->data_off;
/* ... */

seg = seg->next;
}
return m2;
}


>> +} while ((md = md->next) != NULL);
>> +
>> +*prev = NULL;

why this?

>> +__rte_mbuf_sanity_check(mc, 1);
>> +return mc;
>> +}
>> +

I agree this kind of function could be useful. But if there is no
user of this code inside the dpdk, I think we must at least have
a test function for it in app/test/test_mbuf.c


Regards,
Olivier

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Mrzyglod, DanielX T]

>-Original Message-
>From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
>Sent: Friday, July 10, 2015 1:38 AM
>To: dev at dpdk.org
>Cc: Mike Davison ; Stephen Hemminger
>
>Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
>
>From: Stephen Hemminger 
>
>Added rte_pktmbuf_copy() function since copying multi-part
>segments is common issue and can be problematic.
>
>Signed-off-by: Mike Davison 
>Reviewed-by: Stephen Hemminger 
>---
> lib/librte_mbuf/rte_mbuf.h | 59
>++
> 1 file changed, 59 insertions(+)
>
>diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>index 80419df..f0a543b 100644
>--- a/lib/librte_mbuf/rte_mbuf.h
>+++ b/lib/librte_mbuf/rte_mbuf.h
>@@ -60,6 +60,7 @@
> #include 
> #include 
> #include 
>+#include 
>
> #ifdef __cplusplus
> extern "C" {
>@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const
>struct rte_mbuf *m)
>   return !!(m->nb_segs == 1);
> }
>
>+/*
>+ * Creates a copy of the given packet mbuf.
>+ *
>+ * Walks through all segments of the given packet mbuf, and for each of them:
>+ *  - Creates a new packet mbuf from the given pool.
>+ *  - Copy segment to newly created mbuf.
>+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
>+ * from the original packet mbuf.
>+ *
>+ * @param md
>+ *   The packet mbuf to be copied.
>+ * @param mp
>+ *   The mempool from which the mbufs are allocated.
>+ * @return
>+ *   - The pointer to the new mbuf on success.
>+ *   - NULL if allocation fails.
>+ */
>+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
>+  struct rte_mempool *mp)
>+{
>+  struct rte_mbuf *mc = NULL;
>+  struct rte_mbuf **prev = 
>+
>+  do {
>+  struct rte_mbuf *mi;
>+
>+  mi = rte_pktmbuf_alloc(mp);
>+  if (unlikely(mi == NULL)) {
>+  rte_pktmbuf_free(mc);
>+  return NULL;
>+  }
>+
>+  mi->data_off = md->data_off;
>+  mi->data_len = md->data_len;
>+  mi->port = md->port;
>+  mi->vlan_tci = md->vlan_tci;
>+  mi->tx_offload = md->tx_offload;
>+  mi->hash = md->hash;
>+
>+  mi->next = NULL;
>+  mi->pkt_len = md->pkt_len;
>+  mi->nb_segs = md->nb_segs;
>+  mi->ol_flags = md->ol_flags;
>+  mi->packet_type = md->packet_type;
>+
>+  rte_memcpy(rte_pktmbuf_mtod(mi, char *),
>+ rte_pktmbuf_mtod(md, char *),
>+ md->data_len);
>+
>+  *prev = mi;
>+  prev = >next;
>+  } while ((md = md->next) != NULL);
>+
>+  *prev = NULL;
>+  __rte_mbuf_sanity_check(mc, 1);
>+  return mc;
>+}
>+
> /**
>  * Dump an mbuf structure to the console.
>  *
>--
>2.1.4

Hi Stephen :>
This patch look useful in case of backup buffs. 
There will be second approach ?

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Fri, 22 Jan 2016 13:40:44 +
"Mrzyglod, DanielX T"  wrote:

> 
> 
> >-Original Message-
> >From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> >Sent: Friday, July 10, 2015 1:38 AM
> >To: dev at dpdk.org
> >Cc: Mike Davison ; Stephen Hemminger
> >
> >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> >
> >From: Stephen Hemminger 
> >
> >Added rte_pktmbuf_copy() function since copying multi-part
> >segments is common issue and can be problematic.
> >
> >Signed-off-by: Mike Davison 
> >Reviewed-by: Stephen Hemminger 
> >---
> > lib/librte_mbuf/rte_mbuf.h | 59
> >++
> > 1 file changed, 59 insertions(+)
> >
> >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >index 80419df..f0a543b 100644
> >--- a/lib/librte_mbuf/rte_mbuf.h
> >+++ b/lib/librte_mbuf/rte_mbuf.h
> >@@ -60,6 +60,7 @@
> > #include 
> > #include 
> > #include 
> >+#include 
> >
> > #ifdef __cplusplus
> > extern "C" {
> >@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const
> >struct rte_mbuf *m)
> > return !!(m->nb_segs == 1);
> > }
> >
> >+/*
> >+ * Creates a copy of the given packet mbuf.
> >+ *
> >+ * Walks through all segments of the given packet mbuf, and for each of 
> >them:
> >+ *  - Creates a new packet mbuf from the given pool.
> >+ *  - Copy segment to newly created mbuf.
> >+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
> >+ * from the original packet mbuf.
> >+ *
> >+ * @param md
> >+ *   The packet mbuf to be copied.
> >+ * @param mp
> >+ *   The mempool from which the mbufs are allocated.
> >+ * @return
> >+ *   - The pointer to the new mbuf on success.
> >+ *   - NULL if allocation fails.
> >+ */
> >+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
> >+struct rte_mempool *mp)
> >+{
> >+struct rte_mbuf *mc = NULL;
> >+struct rte_mbuf **prev = 
> >+
> >+do {
> >+struct rte_mbuf *mi;
> >+
> >+mi = rte_pktmbuf_alloc(mp);
> >+if (unlikely(mi == NULL)) {
> >+rte_pktmbuf_free(mc);
> >+return NULL;
> >+}
> >+
> >+mi->data_off = md->data_off;
> >+mi->data_len = md->data_len;
> >+mi->port = md->port;
> >+mi->vlan_tci = md->vlan_tci;
> >+mi->tx_offload = md->tx_offload;
> >+mi->hash = md->hash;
> >+
> >+mi->next = NULL;
> >+mi->pkt_len = md->pkt_len;
> >+mi->nb_segs = md->nb_segs;
> >+mi->ol_flags = md->ol_flags;
> >+mi->packet_type = md->packet_type;
> >+
> >+rte_memcpy(rte_pktmbuf_mtod(mi, char *),
> >+   rte_pktmbuf_mtod(md, char *),
> >+   md->data_len);
> >+
> >+*prev = mi;
> >+prev = >next;
> >+} while ((md = md->next) != NULL);
> >+
> >+*prev = NULL;
> >+__rte_mbuf_sanity_check(mc, 1);
> >+return mc;
> >+}
> >+
> > /**
> >  * Dump an mbuf structure to the console.
> >  *
> >--
> >2.1.4
> 
> Hi Stephen :>
> This patch look useful in case of backup buffs. 
> There will be second approach ?

I did bother resubmitting it since unless there are users in current
code base it would likely rot.

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Olivier MATZ]

On 07/10/2015 01:37 AM, Stephen Hemminger wrote:
> From: Stephen Hemminger 
>
> Added rte_pktmbuf_copy() function since copying multi-part
> segments is common issue and can be problematic.
>
> Signed-off-by: Mike Davison 
> Reviewed-by: Stephen Hemminger 
> ---
>   lib/librte_mbuf/rte_mbuf.h | 59 
> ++
>   1 file changed, 59 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 80419df..f0a543b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -60,6 +60,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>
>   #ifdef __cplusplus
>   extern "C" {
> @@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const 
> struct rte_mbuf *m)
>   return !!(m->nb_segs == 1);
>   }
>
> +/*
> + * Creates a copy of the given packet mbuf.
> + *
> + * Walks through all segments of the given packet mbuf, and for each of them:
> + *  - Creates a new packet mbuf from the given pool.
> + *  - Copy segment to newly created mbuf.
> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values
> + * from the original packet mbuf.
> + *
> + * @param md
> + *   The packet mbuf to be copied.
> + * @param mp
> + *   The mempool from which the mbufs are allocated.
> + * @return
> + *   - The pointer to the new mbuf on success.
> + *   - NULL if allocation fails.
> + */
> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
> + struct rte_mempool *mp)
> +{
> + struct rte_mbuf *mc = NULL;
> + struct rte_mbuf **prev = 
> +
> + do {
> + struct rte_mbuf *mi;
> +
> + mi = rte_pktmbuf_alloc(mp);
> + if (unlikely(mi == NULL)) {
> + rte_pktmbuf_free(mc);
> + return NULL;
> + }
> +

I think we should check that the destination mbuf is large enough
to store the segment data. Then we have 2 options on failure:
- split the original segment into several destination segments
- return an error


> + mi->data_off = md->data_off;
> + mi->data_len = md->data_len;
> + mi->port = md->port;
> + mi->vlan_tci = md->vlan_tci;
> + mi->tx_offload = md->tx_offload;
> + mi->hash = md->hash;
> +
> + mi->next = NULL;
> + mi->pkt_len = md->pkt_len;
> + mi->nb_segs = md->nb_segs;
> + mi->ol_flags = md->ol_flags;
> + mi->packet_type = md->packet_type;
> +
> + rte_memcpy(rte_pktmbuf_mtod(mi, char *),
> +rte_pktmbuf_mtod(md, char *),
> +md->data_len);
> +
> + *prev = mi;
> + prev = >next;
> + } while ((md = md->next) != NULL);
> +
> + *prev = NULL;
> + __rte_mbuf_sanity_check(mc, 1);
> + return mc;
> +}
> +
>   /**
>* Dump an mbuf structure to the console.
>*
>

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

From: Stephen Hemminger 

Added rte_pktmbuf_copy() function since copying multi-part
segments is common issue and can be problematic.

Signed-off-by: Mike Davison 
Reviewed-by: Stephen Hemminger 
---
 lib/librte_mbuf/rte_mbuf.h | 59 ++
 1 file changed, 59 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 80419df..f0a543b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -60,6 +60,7 @@
 #include 
 #include 
 #include 
+#include 

 #ifdef __cplusplus
 extern "C" {
@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const struct 
rte_mbuf *m)
return !!(m->nb_segs == 1);
 }

+/*
+ * Creates a copy of the given packet mbuf.
+ *
+ * Walks through all segments of the given packet mbuf, and for each of them:
+ *  - Creates a new packet mbuf from the given pool.
+ *  - Copy segment to newly created mbuf.
+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
+ * from the original packet mbuf.
+ *
+ * @param md
+ *   The packet mbuf to be copied.
+ * @param mp
+ *   The mempool from which the mbufs are allocated.
+ * @return
+ *   - The pointer to the new mbuf on success.
+ *   - NULL if allocation fails.
+ */
+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
+   struct rte_mempool *mp)
+{
+   struct rte_mbuf *mc = NULL;
+   struct rte_mbuf **prev = 
+
+   do {
+   struct rte_mbuf *mi;
+
+   mi = rte_pktmbuf_alloc(mp);
+   if (unlikely(mi == NULL)) {
+   rte_pktmbuf_free(mc);
+   return NULL;
+   }
+
+   mi->data_off = md->data_off;
+   mi->data_len = md->data_len;
+   mi->port = md->port;
+   mi->vlan_tci = md->vlan_tci;
+   mi->tx_offload = md->tx_offload;
+   mi->hash = md->hash;
+
+   mi->next = NULL;
+   mi->pkt_len = md->pkt_len;
+   mi->nb_segs = md->nb_segs;
+   mi->ol_flags = md->ol_flags;
+   mi->packet_type = md->packet_type;
+
+   rte_memcpy(rte_pktmbuf_mtod(mi, char *),
+  rte_pktmbuf_mtod(md, char *),
+  md->data_len);
+
+   *prev = mi;
+   prev = >next;
+   } while ((md = md->next) != NULL);
+
+   *prev = NULL;
+   __rte_mbuf_sanity_check(mc, 1);
+   return mc;
+}
+
 /**
  * Dump an mbuf structure to the console.
  *
-- 
2.1.4