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

  From: Stephen Hemminger <shemming@brocade.com>

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

Signed-off-by: Mike Davison <mdavison@brocade.com>
Reviewed-by: Stephen Hemminger <shemming@brocade.com>
---
 lib/librte_mbuf/rte_mbuf.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)
  

On 07/10/2015 01:37 AM, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
>
> Added rte_pktmbuf_copy() function since copying multi-part
> segments is common issue and can be problematic.
>
> Signed-off-by: Mike Davison <mdavison@brocade.com>
> Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> ---
>   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 <rte_atomic.h>
>   #include <rte_prefetch.h>
>   #include <rte_branch_prediction.h>
> +#include <rte_memcpy.h>
>
>   #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 = &mc;
> +
> +	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 = &mi->next;
> +	} while ((md = md->next) != NULL);
> +
> +	*prev = NULL;
> +	__rte_mbuf_sanity_check(mc, 1);
> +	return mc;
> +}
> +
>   /**
>    * Dump an mbuf structure to the console.
>    *
>
  

>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>Sent: Friday, July 10, 2015 1:38 AM
>To: dev@dpdk.org
>Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
><shemming@brocade.com>
>Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
>
>From: Stephen Hemminger <shemming@brocade.com>
>
>Added rte_pktmbuf_copy() function since copying multi-part
>segments is common issue and can be problematic.
>
>Signed-off-by: Mike Davison <mdavison@brocade.com>
>Reviewed-by: Stephen Hemminger <shemming@brocade.com>
>---
> 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 <rte_atomic.h>
> #include <rte_prefetch.h>
> #include <rte_branch_prediction.h>
>+#include <rte_memcpy.h>
>
> #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 = &mc;
>+
>+	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 = &mi->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 ?
  

On Fri, 22 Jan 2016 13:40:44 +0000
"Mrzyglod, DanielX T" <danielx.t.mrzyglod@intel.com> wrote:

> 
> 
> >-----Original Message-----
> >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> >Sent: Friday, July 10, 2015 1:38 AM
> >To: dev@dpdk.org
> >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> ><shemming@brocade.com>
> >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> >
> >From: Stephen Hemminger <shemming@brocade.com>
> >
> >Added rte_pktmbuf_copy() function since copying multi-part
> >segments is common issue and can be problematic.
> >
> >Signed-off-by: Mike Davison <mdavison@brocade.com>
> >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> >---
> > 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 <rte_atomic.h>
> > #include <rte_prefetch.h>
> > #include <rte_branch_prediction.h>
> >+#include <rte_memcpy.h>
> >
> > #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 = &mc;
> >+
> >+	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 = &mi->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.
  

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 = &mc;
>> +
>> +	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 = &mi->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
  

On Wed, 3 Feb 2016 17:23:05 +0000
Olivier MATZ <olivier.matz@6wind.com> wrote:

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

Hi,

> > >-----Original Message-----
> > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > >Hemminger
> > >Sent: Friday, July 10, 2015 1:38 AM
> > >To: dev@dpdk.org
> > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> > ><shemming@brocade.com>
> > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > >
> > >From: Stephen Hemminger <shemming@brocade.com>
> > >
> > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > >is common issue and can be problematic.
> > >
> > >Signed-off-by: Mike Davison <mdavison@brocade.com>
> > >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> > >---
> >
> > 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
  

On Fri, 18 Mar 2016 17:03:51 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> Hi,
> 
> > > >-----Original Message-----
> > > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > > >Hemminger
> > > >Sent: Friday, July 10, 2015 1:38 AM
> > > >To: dev@dpdk.org
> > > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> > > ><shemming@brocade.com>
> > > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > > >
> > > >From: Stephen Hemminger <shemming@brocade.com>
> > > >
> > > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > > >is common issue and can be problematic.
> > > >
> > > >Signed-off-by: Mike Davison <mdavison@brocade.com>
> > > >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> > > >---
> > >
> > > 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.