diff mbox

[dpdk-dev,v3,1/5] mbuf: fix clone support when application uses private mbuf data

Message ID 1427829784-12323-2-git-send-email-zer0@droids-corp.org (mailing list archive)
State Superseded, archived
Headers

Commit Message

Olivier Matz March 31, 2015, 7:23 p.m. UTC 
  From: Olivier Matz <olivier.matz@6wind.com>

Add a new private_size field in mbuf structure that should
be initialized at mbuf pool creation. This field contains the
size of the application private data in mbufs.

Introduce new static inline functions rte_mbuf_from_indirect()
and rte_mbuf_to_baddr() to replace the existing macros, which
take the private size in account when attaching and detaching
mbufs.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test-pmd/testpmd.c     |  1 +
 examples/vhost/main.c      |  4 +--
 lib/librte_mbuf/rte_mbuf.c |  1 +
 lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
 4 files changed, 63 insertions(+), 20 deletions(-)

Comments

Zoltan Kiss April 2, 2015, 2:32 p.m. UTC | #1 
On 31/03/15 20:23, Olivier Matz wrote:
> From: Olivier Matz <olivier.matz@6wind.com>
>
> Add a new private_size field in mbuf structure that should
> be initialized at mbuf pool creation. This field contains the
> size of the application private data in mbufs.
>
> Introduce new static inline functions rte_mbuf_from_indirect()
> and rte_mbuf_to_baddr() to replace the existing macros, which
> take the private size in account when attaching and detaching
> mbufs.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Reviewed-by: Zoltan Kiss <zoltan.kiss@linaro.org>

I assume the rest of the series haven't changed apart from occasional 
rebasing, I've reviewed them earlier.

> ---
>   app/test-pmd/testpmd.c     |  1 +
>   examples/vhost/main.c      |  4 +--
>   lib/librte_mbuf/rte_mbuf.c |  1 +
>   lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
>   4 files changed, 63 insertions(+), 20 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 3057791..c5a195a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
>   	mb->tx_offload   = 0;
>   	mb->vlan_tci     = 0;
>   	mb->hash.rss     = 0;
> +	mb->priv_size    = 0;
>   }
>
>   static void
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index c3fcb80..e44e82f 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -139,7 +139,7 @@
>   /* Number of descriptors per cacheline. */
>   #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
>
> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
>
>   /* mask of enabled ports */
>   static uint32_t enabled_port_mask = 0;
> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
>   static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
>   {
>   	const struct rte_mempool *mp = m->pool;
> -	void *buf = RTE_MBUF_TO_BADDR(m);
> +	void *buf = rte_mbuf_to_baddr(m);
>   	uint32_t buf_ofs;
>   	uint32_t buf_len = mp->elt_size - sizeof(*m);
>   	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 526b18d..e095999 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>   	m->pool = mp;
>   	m->nb_segs = 1;
>   	m->port = 0xff;
> +	m->priv_size = 0;
>   }
>
>   /* do some sanity checks on a mbuf: panic if it fails */
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..932fe58 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -317,18 +317,51 @@ struct rte_mbuf {
>   			/* uint64_t unused:8; */
>   		};
>   	};
> +
> +	/** Size of the application private data. In case of an indirect
> +	 * mbuf, it stores the direct mbuf private data size. */
> +	uint16_t priv_size;
>   } __rte_cache_aligned;
>
>   /**
> - * Given the buf_addr returns the pointer to corresponding mbuf.
> + * Return the mbuf owning the data buffer address of an indirect mbuf.
> + *
> + * @param mi
> + *   The pointer to the indirect mbuf.
> + * @return
> + *   The address of the direct mbuf corresponding to buffer_addr.
>    */
> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
> +static inline struct rte_mbuf *
> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
> +{
> +       struct rte_mbuf *md;
> +
> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
> +	* private size of the direct mbuf */
> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
> +	       mi->priv_size);
> +       return md;
> +}
>
>   /**
> - * Given the pointer to mbuf returns an address where it's  buf_addr
> - * should point to.
> + * Return the buffer address embedded in the given mbuf.
> + *
> + * The user must ensure that m->priv_size corresponds to the
> + * private size of this mbuf, which is not the case for indirect
> + * mbufs.
> + *
> + * @param md
> + *   The pointer to the mbuf.
> + * @return
> + *   The address of the data buffer owned by the mbuf.
>    */
> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
> +static inline char *
> +rte_mbuf_to_baddr(struct rte_mbuf *md)
> +{
> +       char *buffer_addr;
> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
> +       return buffer_addr;
> +}
>
>   /**
>    * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>
>   /**
>    * Attach packet mbuf to another packet mbuf.
> + *
>    * After attachment we refer the mbuf we attached as 'indirect',
>    * while mbuf we attached to as 'direct'.
>    * Right now, not supported:
> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>    * @param md
>    *   The direct packet mbuf.
>    */
> -
>   static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>   {
>   	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>   	mi->buf_physaddr = md->buf_physaddr;
>   	mi->buf_addr = md->buf_addr;
>   	mi->buf_len = md->buf_len;
> +	mi->priv_size = md->priv_size;
>
>   	mi->next = md->next;
>   	mi->data_off = md->data_off;
> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>   }
>
>   /**
> - * Detach an indirect packet mbuf -
> + * Detach an indirect packet mbuf.
> + *
>    *  - restore original mbuf address and length values.
>    *  - reset pktmbuf data and data_len to their default values.
>    *  All other fields of the given packet mbuf will be left intact.
> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>    * @param m
>    *   The indirect attached packet mbuf.
>    */
> -
>   static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>   {
> -	const struct rte_mempool *mp = m->pool;
> -	void *buf = RTE_MBUF_TO_BADDR(m);
> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> -
> +	struct rte_pktmbuf_pool_private *mbp_priv;
> +	struct rte_mempool *mp = m->pool;
> +	void *buf;
> +	unsigned mhdr_size;
> +
> +	/* first, restore the priv_size, this is needed before calling
> +	 * rte_mbuf_to_baddr() */
> +	mbp_priv = rte_mempool_get_priv(mp);
> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
> +		mbp_priv->mbuf_data_room_size -
> +		sizeof(struct rte_mbuf);
> +
> +	buf = rte_mbuf_to_baddr(m);
> +	mhdr_size = (char *)buf - (char *)m;
> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
>   	m->buf_addr = buf;
> -	m->buf_len = (uint16_t)buf_len;
> -
> +	m->buf_len = (uint16_t)(mp->elt_size - mhdr_size);
>   	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
>   			RTE_PKTMBUF_HEADROOM : m->buf_len;
> -
>   	m->data_len = 0;
> -
>   	m->ol_flags = 0;
>   }
>
> @@ -774,7 +815,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>   		 *  - free attached mbuf segment
>   		 */
>   		if (RTE_MBUF_INDIRECT(m)) {
> -			struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr);
> +			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>   			rte_pktmbuf_detach(m);
>   			if (rte_mbuf_refcnt_update(md, -1) == 0)
>   				__rte_mbuf_raw_free(md);
>
Ananyev, Konstantin April 2, 2015, 5:21 p.m. UTC | #2 
Hi Olivier,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, March 31, 2015 8:23 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; Olivier Matz
> Subject: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> 
> From: Olivier Matz <olivier.matz@6wind.com>
> 
> Add a new private_size field in mbuf structure that should
> be initialized at mbuf pool creation. This field contains the
> size of the application private data in mbufs.
> 
> Introduce new static inline functions rte_mbuf_from_indirect()
> and rte_mbuf_to_baddr() to replace the existing macros, which
> take the private size in account when attaching and detaching
> mbufs.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  app/test-pmd/testpmd.c     |  1 +
>  examples/vhost/main.c      |  4 +--
>  lib/librte_mbuf/rte_mbuf.c |  1 +
>  lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
>  4 files changed, 63 insertions(+), 20 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 3057791..c5a195a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
>  	mb->tx_offload   = 0;
>  	mb->vlan_tci     = 0;
>  	mb->hash.rss     = 0;
> +	mb->priv_size    = 0;
>  }
> 
>  static void
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index c3fcb80..e44e82f 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -139,7 +139,7 @@
>  /* Number of descriptors per cacheline. */
>  #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
> 
> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
> 
>  /* mask of enabled ports */
>  static uint32_t enabled_port_mask = 0;
> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
>  static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
>  {
>  	const struct rte_mempool *mp = m->pool;
> -	void *buf = RTE_MBUF_TO_BADDR(m);
> +	void *buf = rte_mbuf_to_baddr(m);
>  	uint32_t buf_ofs;
>  	uint32_t buf_len = mp->elt_size - sizeof(*m);
>  	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 526b18d..e095999 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>  	m->pool = mp;
>  	m->nb_segs = 1;
>  	m->port = 0xff;
> +	m->priv_size = 0;

Why it is 0?
Shouldn't it be the same calulations as in detach() below:
m->priv_size = /*get private size from mempool private*/;
m->buf_addr = (char *)m + sizeof(struct rte_mbuf) + m->priv_size;
m->buf_len = mp->elt_size - sizeof(struct rte_mbuf) - m->priv_size;
?

BTW, don't see changes in rte_pktmbuf_pool_init() to setup
mbp_priv->mbuf_data_room_size properly.
Without that changes, how can people start using that feature?
It seems that the only way now - setup priv_size and buf_len for each mbuf manually.

>  }
> 
>  /* do some sanity checks on a mbuf: panic if it fails */
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..932fe58 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -317,18 +317,51 @@ struct rte_mbuf {
>  			/* uint64_t unused:8; */
>  		};
>  	};
> +
> +	/** Size of the application private data. In case of an indirect
> +	 * mbuf, it stores the direct mbuf private data size. */
> +	uint16_t priv_size;
>  } __rte_cache_aligned;
> 
>  /**
> - * Given the buf_addr returns the pointer to corresponding mbuf.
> + * Return the mbuf owning the data buffer address of an indirect mbuf.
> + *
> + * @param mi
> + *   The pointer to the indirect mbuf.
> + * @return
> + *   The address of the direct mbuf corresponding to buffer_addr.
>   */
> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
> +static inline struct rte_mbuf *
> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
> +{
> +       struct rte_mbuf *md;
> +
> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
> +	* private size of the direct mbuf */
> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
> +	       mi->priv_size);

(uintptr_t)mi->buf_addr?

> +       return md;
> +}
> 
>  /**
> - * Given the pointer to mbuf returns an address where it's  buf_addr
> - * should point to.
> + * Return the buffer address embedded in the given mbuf.
> + *
> + * The user must ensure that m->priv_size corresponds to the
> + * private size of this mbuf, which is not the case for indirect
> + * mbufs.
> + *
> + * @param md
> + *   The pointer to the mbuf.
> + * @return
> + *   The address of the data buffer owned by the mbuf.
>   */
> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
> +static inline char *

Might be better to return 'void *' here.

> +rte_mbuf_to_baddr(struct rte_mbuf *md)
> +{
> +       char *buffer_addr;

uintptr_t buffer_addr? 

> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
> +       return buffer_addr;
> +}
> 
>  /**
>   * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> 
>  /**
>   * Attach packet mbuf to another packet mbuf.
> + *
>   * After attachment we refer the mbuf we attached as 'indirect',
>   * while mbuf we attached to as 'direct'.
>   * Right now, not supported:
> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>   * @param md
>   *   The direct packet mbuf.
>   */
> -
>  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>  {
>  	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>  	mi->buf_physaddr = md->buf_physaddr;
>  	mi->buf_addr = md->buf_addr;
>  	mi->buf_len = md->buf_len;
> +	mi->priv_size = md->priv_size;
> 
>  	mi->next = md->next;
>  	mi->data_off = md->data_off;
> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>  }
> 
>  /**
> - * Detach an indirect packet mbuf -
> + * Detach an indirect packet mbuf.
> + *
>   *  - restore original mbuf address and length values.
>   *  - reset pktmbuf data and data_len to their default values.
>   *  All other fields of the given packet mbuf will be left intact.
> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>   * @param m
>   *   The indirect attached packet mbuf.
>   */
> -
>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  {
> -	const struct rte_mempool *mp = m->pool;
> -	void *buf = RTE_MBUF_TO_BADDR(m);
> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> -
> +	struct rte_pktmbuf_pool_private *mbp_priv;
> +	struct rte_mempool *mp = m->pool;
> +	void *buf;
> +	unsigned mhdr_size;
> +
> +	/* first, restore the priv_size, this is needed before calling
> +	 * rte_mbuf_to_baddr() */
> +	mbp_priv = rte_mempool_get_priv(mp);
> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
> +		mbp_priv->mbuf_data_room_size -
> +		sizeof(struct rte_mbuf);

I think it is better to put this priv_size calculation above into the separate function -
rte_mbuf_get_priv_size(m) or something.
We need it in few places, and users would probably need it anyway. 

> +
> +	buf = rte_mbuf_to_baddr(m);
> +	mhdr_size = (char *)buf - (char *)m;

Why do you need to recalculate mhdr_size here?
As I understand it is a m->priv_size, and you just retrieved it, 2 lines above.

> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;

Actually I think could just be:
m->buf_physaddr = rte_mempool_virt2phy(mp, buf);

>  	m->buf_addr = buf;
> -	m->buf_len = (uint16_t)buf_len;
> -
> +	m->buf_len = (uint16_t)(mp->elt_size - mhdr_size);
>  	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
>  			RTE_PKTMBUF_HEADROOM : m->buf_len;
> -
>  	m->data_len = 0;
> -
>  	m->ol_flags = 0;
>  }
> 
> @@ -774,7 +815,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  		 *  - free attached mbuf segment
>  		 */
>  		if (RTE_MBUF_INDIRECT(m)) {
> -			struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr);
> +			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
>  			rte_pktmbuf_detach(m);
>  			if (rte_mbuf_refcnt_update(md, -1) == 0)
>  				__rte_mbuf_raw_free(md);
> --
> 2.1.4
Olivier Matz April 6, 2015, 9:49 p.m. UTC | #3 
Hi Konstantin,

Thanks for your comments.

On 04/02/2015 07:21 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
> 
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, March 31, 2015 8:23 PM
>> To: dev@dpdk.org
>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; Olivier Matz
>> Subject: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
>>
>> From: Olivier Matz <olivier.matz@6wind.com>
>>
>> Add a new private_size field in mbuf structure that should
>> be initialized at mbuf pool creation. This field contains the
>> size of the application private data in mbufs.
>>
>> Introduce new static inline functions rte_mbuf_from_indirect()
>> and rte_mbuf_to_baddr() to replace the existing macros, which
>> take the private size in account when attaching and detaching
>> mbufs.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  app/test-pmd/testpmd.c     |  1 +
>>  examples/vhost/main.c      |  4 +--
>>  lib/librte_mbuf/rte_mbuf.c |  1 +
>>  lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
>>  4 files changed, 63 insertions(+), 20 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 3057791..c5a195a 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
>>  	mb->tx_offload   = 0;
>>  	mb->vlan_tci     = 0;
>>  	mb->hash.rss     = 0;
>> +	mb->priv_size    = 0;
>>  }
>>
>>  static void
>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
>> index c3fcb80..e44e82f 100644
>> --- a/examples/vhost/main.c
>> +++ b/examples/vhost/main.c
>> @@ -139,7 +139,7 @@
>>  /* Number of descriptors per cacheline. */
>>  #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
>>
>> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
>> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
>>
>>  /* mask of enabled ports */
>>  static uint32_t enabled_port_mask = 0;
>> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
>>  static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
>>  {
>>  	const struct rte_mempool *mp = m->pool;
>> -	void *buf = RTE_MBUF_TO_BADDR(m);
>> +	void *buf = rte_mbuf_to_baddr(m);
>>  	uint32_t buf_ofs;
>>  	uint32_t buf_len = mp->elt_size - sizeof(*m);
>>  	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>> index 526b18d..e095999 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>>  	m->pool = mp;
>>  	m->nb_segs = 1;
>>  	m->port = 0xff;
>> +	m->priv_size = 0;
> 
> Why it is 0?
> Shouldn't it be the same calulations as in detach() below:
> m->priv_size = /*get private size from mempool private*/;
> m->buf_addr = (char *)m + sizeof(struct rte_mbuf) + m->priv_size;
> m->buf_len = mp->elt_size - sizeof(struct rte_mbuf) - m->priv_size;
> ?

It's 0 because we also have in the function (not visible in the
patch):

  m->buf_addr = (char *)m + sizeof(struct rte_mbuf);

It means that an application that wants to use a private area has
to provide another init function derived from this default function.
This was already the case before the patch series.

As we discussed in previous mail, I plan to propose a rework of
mbuf pool initialization in another series, and my initial idea was to
change this at the same time. But on the other hand it does not hurt
to do this change now. I'll include it in next version.


> BTW, don't see changes in rte_pktmbuf_pool_init() to setup
> mbp_priv->mbuf_data_room_size properly.
> Without that changes, how can people start using that feature?
> It seems that the only way now - setup priv_size and buf_len for each mbuf manually.

It's the same reason than above. To use a private are, the user has
to provide its own function that sets up data_room_size, derived from
this pool_init default function. This was also the case before the
patch series.


> 
>>  }
>>
>>  /* do some sanity checks on a mbuf: panic if it fails */
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 17ba791..932fe58 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -317,18 +317,51 @@ struct rte_mbuf {
>>  			/* uint64_t unused:8; */
>>  		};
>>  	};
>> +
>> +	/** Size of the application private data. In case of an indirect
>> +	 * mbuf, it stores the direct mbuf private data size. */
>> +	uint16_t priv_size;
>>  } __rte_cache_aligned;
>>
>>  /**
>> - * Given the buf_addr returns the pointer to corresponding mbuf.
>> + * Return the mbuf owning the data buffer address of an indirect mbuf.
>> + *
>> + * @param mi
>> + *   The pointer to the indirect mbuf.
>> + * @return
>> + *   The address of the direct mbuf corresponding to buffer_addr.
>>   */
>> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>> +static inline struct rte_mbuf *
>> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
>> +{
>> +       struct rte_mbuf *md;
>> +
>> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
>> +	* private size of the direct mbuf */
>> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
>> +	       mi->priv_size);
> 
> (uintptr_t)mi->buf_addr?

Any clue why (uintptr_t) would be better than (char *) ?
By the way, I added this cast because it would not compile with
g++ (and probably with icc too).

> 
>> +       return md;
>> +}
>>
>>  /**
>> - * Given the pointer to mbuf returns an address where it's  buf_addr
>> - * should point to.
>> + * Return the buffer address embedded in the given mbuf.
>> + *
>> + * The user must ensure that m->priv_size corresponds to the
>> + * private size of this mbuf, which is not the case for indirect
>> + * mbufs.
>> + *
>> + * @param md
>> + *   The pointer to the mbuf.
>> + * @return
>> + *   The address of the data buffer owned by the mbuf.
>>   */
>> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>> +static inline char *
> 
> Might be better to return 'void *' here.

Ok, as m->buf_addr is a (void *).

> 
>> +rte_mbuf_to_baddr(struct rte_mbuf *md)
>> +{
>> +       char *buffer_addr;
> 
> uintptr_t buffer_addr? 

Same question than above, I don't really see why it's better than
(char *).

> 
>> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
>> +       return buffer_addr;
>> +}
>>
>>  /**
>>   * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
>> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>
>>  /**
>>   * Attach packet mbuf to another packet mbuf.
>> + *
>>   * After attachment we refer the mbuf we attached as 'indirect',
>>   * while mbuf we attached to as 'direct'.
>>   * Right now, not supported:
>> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>   * @param md
>>   *   The direct packet mbuf.
>>   */
>> -
>>  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>  {
>>  	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
>> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>  	mi->buf_physaddr = md->buf_physaddr;
>>  	mi->buf_addr = md->buf_addr;
>>  	mi->buf_len = md->buf_len;
>> +	mi->priv_size = md->priv_size;
>>
>>  	mi->next = md->next;
>>  	mi->data_off = md->data_off;
>> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>  }
>>
>>  /**
>> - * Detach an indirect packet mbuf -
>> + * Detach an indirect packet mbuf.
>> + *
>>   *  - restore original mbuf address and length values.
>>   *  - reset pktmbuf data and data_len to their default values.
>>   *  All other fields of the given packet mbuf will be left intact.
>> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>   * @param m
>>   *   The indirect attached packet mbuf.
>>   */
>> -
>>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>  {
>> -	const struct rte_mempool *mp = m->pool;
>> -	void *buf = RTE_MBUF_TO_BADDR(m);
>> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
>> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
>> -
>> +	struct rte_pktmbuf_pool_private *mbp_priv;
>> +	struct rte_mempool *mp = m->pool;
>> +	void *buf;
>> +	unsigned mhdr_size;
>> +
>> +	/* first, restore the priv_size, this is needed before calling
>> +	 * rte_mbuf_to_baddr() */
>> +	mbp_priv = rte_mempool_get_priv(mp);
>> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
>> +		mbp_priv->mbuf_data_room_size -
>> +		sizeof(struct rte_mbuf);
> 
> I think it is better to put this priv_size calculation above into the separate function -
> rte_mbuf_get_priv_size(m) or something.
> We need it in few places, and users would probably need it anyway.

yep, good idea

> 
>> +
>> +	buf = rte_mbuf_to_baddr(m);
>> +	mhdr_size = (char *)buf - (char *)m;
> 
> Why do you need to recalculate mhdr_size here?
> As I understand it is a m->priv_size, and you just retrieved it, 2 lines above.
> 

It's not m->priv_size but (sizeof(rte_mbuf) + m->priv_size).
In both case, it requires an operation, but maybe
  mhdr_size = (sizeof(rte_mbuf) + m->priv_size)
is clearer than
  mhdr_size = (char *)buf - (char *)m


>> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
> 
> Actually I think could just be:
> m->buf_physaddr = rte_mempool_virt2phy(mp, buf);

Even if it would work, the API of rte_mempool_virt2phy()
says that the second argument should be "A pointer (virtual address)
to the element of the pool."
I think we should keep the initial code.

Regards,
Olivier
Ananyev, Konstantin April 7, 2015, 12:40 p.m. UTC | #4 
Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, April 06, 2015 10:50 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> 
> Hi Konstantin,
> 
> Thanks for your comments.
> 
> On 04/02/2015 07:21 PM, Ananyev, Konstantin wrote:
> > Hi Olivier,
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Tuesday, March 31, 2015 8:23 PM
> >> To: dev@dpdk.org
> >> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; Olivier Matz
> >> Subject: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> >>
> >> From: Olivier Matz <olivier.matz@6wind.com>
> >>
> >> Add a new private_size field in mbuf structure that should
> >> be initialized at mbuf pool creation. This field contains the
> >> size of the application private data in mbufs.
> >>
> >> Introduce new static inline functions rte_mbuf_from_indirect()
> >> and rte_mbuf_to_baddr() to replace the existing macros, which
> >> take the private size in account when attaching and detaching
> >> mbufs.
> >>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> ---
> >>  app/test-pmd/testpmd.c     |  1 +
> >>  examples/vhost/main.c      |  4 +--
> >>  lib/librte_mbuf/rte_mbuf.c |  1 +
> >>  lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
> >>  4 files changed, 63 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >> index 3057791..c5a195a 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
> >>  	mb->tx_offload   = 0;
> >>  	mb->vlan_tci     = 0;
> >>  	mb->hash.rss     = 0;
> >> +	mb->priv_size    = 0;
> >>  }
> >>
> >>  static void
> >> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> >> index c3fcb80..e44e82f 100644
> >> --- a/examples/vhost/main.c
> >> +++ b/examples/vhost/main.c
> >> @@ -139,7 +139,7 @@
> >>  /* Number of descriptors per cacheline. */
> >>  #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
> >>
> >> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
> >> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
> >>
> >>  /* mask of enabled ports */
> >>  static uint32_t enabled_port_mask = 0;
> >> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
> >>  static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
> >>  {
> >>  	const struct rte_mempool *mp = m->pool;
> >> -	void *buf = RTE_MBUF_TO_BADDR(m);
> >> +	void *buf = rte_mbuf_to_baddr(m);
> >>  	uint32_t buf_ofs;
> >>  	uint32_t buf_len = mp->elt_size - sizeof(*m);
> >>  	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
> >> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> >> index 526b18d..e095999 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.c
> >> +++ b/lib/librte_mbuf/rte_mbuf.c
> >> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
> >>  	m->pool = mp;
> >>  	m->nb_segs = 1;
> >>  	m->port = 0xff;
> >> +	m->priv_size = 0;
> >
> > Why it is 0?
> > Shouldn't it be the same calulations as in detach() below:
> > m->priv_size = /*get private size from mempool private*/;
> > m->buf_addr = (char *)m + sizeof(struct rte_mbuf) + m->priv_size;
> > m->buf_len = mp->elt_size - sizeof(struct rte_mbuf) - m->priv_size;
> > ?
> 
> It's 0 because we also have in the function (not visible in the
> patch):
> 
>   m->buf_addr = (char *)m + sizeof(struct rte_mbuf);

Yep, that's why as I wrote above, I think we need to setup here all 3 fields:
priv_size, buf_addr, buf_len exactly in the same way as in detach().  

> 
> It means that an application that wants to use a private area has
> to provide another init function derived from this default function.

After your changes, attach/free and other functions from public mbuf API
rely on priv_size being set properly.
So I suppose 'official' pktmbuf_init() should also set it in a proper manner. 

> This was already the case before the patch series.

Before this patch series, we don't have priv_size, so we have nothing to setup.

> 
> As we discussed in previous mail, I plan to propose a rework of
> mbuf pool initialization in another series, and my initial idea was to
> change this at the same time. But on the other hand it does not hurt
> to do this change now. I'll include it in next version.

Ok.

> 
> 
> > BTW, don't see changes in rte_pktmbuf_pool_init() to setup
> > mbp_priv->mbuf_data_room_size properly.
> > Without that changes, how can people start using that feature?
> > It seems that the only way now - setup priv_size and buf_len for each mbuf manually.
> 
> It's the same reason than above. To use a private are, the user has
> to provide its own function that sets up data_room_size, derived from
> this pool_init default function. This was also the case before the
> patch series.
> 
> 
> >
> >>  }
> >>
> >>  /* do some sanity checks on a mbuf: panic if it fails */
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index 17ba791..932fe58 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -317,18 +317,51 @@ struct rte_mbuf {
> >>  			/* uint64_t unused:8; */
> >>  		};
> >>  	};
> >> +
> >> +	/** Size of the application private data. In case of an indirect
> >> +	 * mbuf, it stores the direct mbuf private data size. */
> >> +	uint16_t priv_size;
> >>  } __rte_cache_aligned;
> >>
> >>  /**
> >> - * Given the buf_addr returns the pointer to corresponding mbuf.
> >> + * Return the mbuf owning the data buffer address of an indirect mbuf.
> >> + *
> >> + * @param mi
> >> + *   The pointer to the indirect mbuf.
> >> + * @return
> >> + *   The address of the direct mbuf corresponding to buffer_addr.
> >>   */
> >> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
> >> +static inline struct rte_mbuf *
> >> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
> >> +{
> >> +       struct rte_mbuf *md;
> >> +
> >> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
> >> +	* private size of the direct mbuf */
> >> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
> >> +	       mi->priv_size);
> >
> > (uintptr_t)mi->buf_addr?
> 
> Any clue why (uintptr_t) would be better than (char *) ?

No big difference really, just looks a bit better to me :)

> By the way, I added this cast because it would not compile with
> g++ (and probably with icc too).
> 
> >
> >> +       return md;
> >> +}
> >>
> >>  /**
> >> - * Given the pointer to mbuf returns an address where it's  buf_addr
> >> - * should point to.
> >> + * Return the buffer address embedded in the given mbuf.
> >> + *
> >> + * The user must ensure that m->priv_size corresponds to the
> >> + * private size of this mbuf, which is not the case for indirect
> >> + * mbufs.
> >> + *
> >> + * @param md
> >> + *   The pointer to the mbuf.
> >> + * @return
> >> + *   The address of the data buffer owned by the mbuf.
> >>   */
> >> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
> >> +static inline char *
> >
> > Might be better to return 'void *' here.
> 
> Ok, as m->buf_addr is a (void *).
> 
> >
> >> +rte_mbuf_to_baddr(struct rte_mbuf *md)
> >> +{
> >> +       char *buffer_addr;
> >
> > uintptr_t buffer_addr?
> 
> Same question than above, I don't really see why it's better than
> (char *).
> 
> >
> >> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
> >> +       return buffer_addr;
> >> +}
> >>
> >>  /**
> >>   * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> >> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >>
> >>  /**
> >>   * Attach packet mbuf to another packet mbuf.
> >> + *
> >>   * After attachment we refer the mbuf we attached as 'indirect',
> >>   * while mbuf we attached to as 'direct'.
> >>   * Right now, not supported:
> >> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >>   * @param md
> >>   *   The direct packet mbuf.
> >>   */
> >> -
> >>  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>  {
> >>  	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
> >> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>  	mi->buf_physaddr = md->buf_physaddr;
> >>  	mi->buf_addr = md->buf_addr;
> >>  	mi->buf_len = md->buf_len;
> >> +	mi->priv_size = md->priv_size;
> >>
> >>  	mi->next = md->next;
> >>  	mi->data_off = md->data_off;
> >> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>  }
> >>
> >>  /**
> >> - * Detach an indirect packet mbuf -
> >> + * Detach an indirect packet mbuf.
> >> + *
> >>   *  - restore original mbuf address and length values.
> >>   *  - reset pktmbuf data and data_len to their default values.
> >>   *  All other fields of the given packet mbuf will be left intact.
> >> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>   * @param m
> >>   *   The indirect attached packet mbuf.
> >>   */
> >> -
> >>  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> >>  {
> >> -	const struct rte_mempool *mp = m->pool;
> >> -	void *buf = RTE_MBUF_TO_BADDR(m);
> >> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
> >> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> >> -
> >> +	struct rte_pktmbuf_pool_private *mbp_priv;
> >> +	struct rte_mempool *mp = m->pool;
> >> +	void *buf;
> >> +	unsigned mhdr_size;
> >> +
> >> +	/* first, restore the priv_size, this is needed before calling
> >> +	 * rte_mbuf_to_baddr() */
> >> +	mbp_priv = rte_mempool_get_priv(mp);
> >> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
> >> +		mbp_priv->mbuf_data_room_size -
> >> +		sizeof(struct rte_mbuf);
> >
> > I think it is better to put this priv_size calculation above into the separate function -
> > rte_mbuf_get_priv_size(m) or something.
> > We need it in few places, and users would probably need it anyway.
> 
> yep, good idea
> 
> >
> >> +
> >> +	buf = rte_mbuf_to_baddr(m);
> >> +	mhdr_size = (char *)buf - (char *)m;
> >
> > Why do you need to recalculate mhdr_size here?
> > As I understand it is a m->priv_size, and you just retrieved it, 2 lines above.
> >
> 
> It's not m->priv_size but (sizeof(rte_mbuf) + m->priv_size).

Ah yes, sorry for confusion.

> In both case, it requires an operation, but maybe
>   mhdr_size = (sizeof(rte_mbuf) + m->priv_size)
> is clearer than
>   mhdr_size = (char *)buf - (char *)m
> 
> 
> >> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
> >
> > Actually I think could just be:
> > m->buf_physaddr = rte_mempool_virt2phy(mp, buf);
> 
> Even if it would work, the API of rte_mempool_virt2phy()
> says that the second argument should be "A pointer (virtual address)
> to the element of the pool."
> I think we should keep the initial code.

Ok.
Konstantin

> 
> Regards,
> Olivier
>
Olivier Matz April 7, 2015, 3:45 p.m. UTC | #5 
Hi Konstantin,

On 04/07/2015 02:40 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Monday, April 06, 2015 10:50 PM
>> To: Ananyev, Konstantin; dev@dpdk.org
>> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
>> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
>>
>> Hi Konstantin,
>>
>> Thanks for your comments.
>>
>> On 04/02/2015 07:21 PM, Ananyev, Konstantin wrote:
>>> Hi Olivier,
>>>
>>>> -----Original Message-----
>>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>>> Sent: Tuesday, March 31, 2015 8:23 PM
>>>> To: dev@dpdk.org
>>>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; Olivier Matz
>>>> Subject: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
>>>>
>>>> From: Olivier Matz <olivier.matz@6wind.com>
>>>>
>>>> Add a new private_size field in mbuf structure that should
>>>> be initialized at mbuf pool creation. This field contains the
>>>> size of the application private data in mbufs.
>>>>
>>>> Introduce new static inline functions rte_mbuf_from_indirect()
>>>> and rte_mbuf_to_baddr() to replace the existing macros, which
>>>> take the private size in account when attaching and detaching
>>>> mbufs.
>>>>
>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>> ---
>>>>   app/test-pmd/testpmd.c     |  1 +
>>>>   examples/vhost/main.c      |  4 +--
>>>>   lib/librte_mbuf/rte_mbuf.c |  1 +
>>>>   lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
>>>>   4 files changed, 63 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>>> index 3057791..c5a195a 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
>>>>   	mb->tx_offload   = 0;
>>>>   	mb->vlan_tci     = 0;
>>>>   	mb->hash.rss     = 0;
>>>> +	mb->priv_size    = 0;
>>>>   }
>>>>
>>>>   static void
>>>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
>>>> index c3fcb80..e44e82f 100644
>>>> --- a/examples/vhost/main.c
>>>> +++ b/examples/vhost/main.c
>>>> @@ -139,7 +139,7 @@
>>>>   /* Number of descriptors per cacheline. */
>>>>   #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
>>>>
>>>> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
>>>> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
>>>>
>>>>   /* mask of enabled ports */
>>>>   static uint32_t enabled_port_mask = 0;
>>>> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
>>>>   static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
>>>>   {
>>>>   	const struct rte_mempool *mp = m->pool;
>>>> -	void *buf = RTE_MBUF_TO_BADDR(m);
>>>> +	void *buf = rte_mbuf_to_baddr(m);
>>>>   	uint32_t buf_ofs;
>>>>   	uint32_t buf_len = mp->elt_size - sizeof(*m);
>>>>   	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
>>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>>>> index 526b18d..e095999 100644
>>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>>> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>>>>   	m->pool = mp;
>>>>   	m->nb_segs = 1;
>>>>   	m->port = 0xff;
>>>> +	m->priv_size = 0;
>>>
>>> Why it is 0?
>>> Shouldn't it be the same calulations as in detach() below:
>>> m->priv_size = /*get private size from mempool private*/;
>>> m->buf_addr = (char *)m + sizeof(struct rte_mbuf) + m->priv_size;
>>> m->buf_len = mp->elt_size - sizeof(struct rte_mbuf) - m->priv_size;
>>> ?
>>
>> It's 0 because we also have in the function (not visible in the
>> patch):
>>
>>    m->buf_addr = (char *)m + sizeof(struct rte_mbuf);
>
> Yep, that's why as I wrote above, I think we need to setup here all 3 fields:
> priv_size, buf_addr, buf_len exactly in the same way as in detach().
>
>>
>> It means that an application that wants to use a private area has
>> to provide another init function derived from this default function.
>
> After your changes, attach/free and other functions from public mbuf API
> rely on priv_size being set properly.
> So I suppose 'official' pktmbuf_init() should also set it in a proper manner.
>
>> This was already the case before the patch series.
>
> Before this patch series, we don't have priv_size, so we have nothing to setup.
>
>>
>> As we discussed in previous mail, I plan to propose a rework of
>> mbuf pool initialization in another series, and my initial idea was to
>> change this at the same time. But on the other hand it does not hurt
>> to do this change now. I'll include it in next version.
>
> Ok.

Just to be sure we're on the same line:

- before the patch series

   - private area was working before that patch series if clones were not
     used. To use a private are, the user had to provide another
     function derived from pktmbuf_init() to change m->buf_addr and
     m->buf_len.
   - using both private area + clones was broken

- after the patch series

   - private area is working with or without clone. But yo use it,
     the user still has to provide another function to change
     m->buf_addr, m->buf_len *and m->priv_size*.

The series just fixes the fact that "clones + priv" was not working.
It does not address the problem that providing a new pktmbuf_init()
function is required to use privata area. To fix this, I think it
could require a API evolution that should be part of another series.

I'll send a v4 addressing the comments soon, thanks.

Regards,
Olivier



>
>>
>>
>>> BTW, don't see changes in rte_pktmbuf_pool_init() to setup
>>> mbp_priv->mbuf_data_room_size properly.
>>> Without that changes, how can people start using that feature?
>>> It seems that the only way now - setup priv_size and buf_len for each mbuf manually.
>>
>> It's the same reason than above. To use a private are, the user has
>> to provide its own function that sets up data_room_size, derived from
>> this pool_init default function. This was also the case before the
>> patch series.
>>
>>
>>>
>>>>   }
>>>>
>>>>   /* do some sanity checks on a mbuf: panic if it fails */
>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>> index 17ba791..932fe58 100644
>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>> @@ -317,18 +317,51 @@ struct rte_mbuf {
>>>>   			/* uint64_t unused:8; */
>>>>   		};
>>>>   	};
>>>> +
>>>> +	/** Size of the application private data. In case of an indirect
>>>> +	 * mbuf, it stores the direct mbuf private data size. */
>>>> +	uint16_t priv_size;
>>>>   } __rte_cache_aligned;
>>>>
>>>>   /**
>>>> - * Given the buf_addr returns the pointer to corresponding mbuf.
>>>> + * Return the mbuf owning the data buffer address of an indirect mbuf.
>>>> + *
>>>> + * @param mi
>>>> + *   The pointer to the indirect mbuf.
>>>> + * @return
>>>> + *   The address of the direct mbuf corresponding to buffer_addr.
>>>>    */
>>>> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>>>> +static inline struct rte_mbuf *
>>>> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>>> +{
>>>> +       struct rte_mbuf *md;
>>>> +
>>>> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
>>>> +	* private size of the direct mbuf */
>>>> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
>>>> +	       mi->priv_size);
>>>
>>> (uintptr_t)mi->buf_addr?
>>
>> Any clue why (uintptr_t) would be better than (char *) ?
>
> No big difference really, just looks a bit better to me :)
>
>> By the way, I added this cast because it would not compile with
>> g++ (and probably with icc too).
>>
>>>
>>>> +       return md;
>>>> +}
>>>>
>>>>   /**
>>>> - * Given the pointer to mbuf returns an address where it's  buf_addr
>>>> - * should point to.
>>>> + * Return the buffer address embedded in the given mbuf.
>>>> + *
>>>> + * The user must ensure that m->priv_size corresponds to the
>>>> + * private size of this mbuf, which is not the case for indirect
>>>> + * mbufs.
>>>> + *
>>>> + * @param md
>>>> + *   The pointer to the mbuf.
>>>> + * @return
>>>> + *   The address of the data buffer owned by the mbuf.
>>>>    */
>>>> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>>>> +static inline char *
>>>
>>> Might be better to return 'void *' here.
>>
>> Ok, as m->buf_addr is a (void *).
>>
>>>
>>>> +rte_mbuf_to_baddr(struct rte_mbuf *md)
>>>> +{
>>>> +       char *buffer_addr;
>>>
>>> uintptr_t buffer_addr?
>>
>> Same question than above, I don't really see why it's better than
>> (char *).
>>
>>>
>>>> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
>>>> +       return buffer_addr;
>>>> +}
>>>>
>>>>   /**
>>>>    * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
>>>> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>>>
>>>>   /**
>>>>    * Attach packet mbuf to another packet mbuf.
>>>> + *
>>>>    * After attachment we refer the mbuf we attached as 'indirect',
>>>>    * while mbuf we attached to as 'direct'.
>>>>    * Right now, not supported:
>>>> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
>>>>    * @param md
>>>>    *   The direct packet mbuf.
>>>>    */
>>>> -
>>>>   static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>>>   {
>>>>   	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
>>>> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>>>   	mi->buf_physaddr = md->buf_physaddr;
>>>>   	mi->buf_addr = md->buf_addr;
>>>>   	mi->buf_len = md->buf_len;
>>>> +	mi->priv_size = md->priv_size;
>>>>
>>>>   	mi->next = md->next;
>>>>   	mi->data_off = md->data_off;
>>>> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>>>   }
>>>>
>>>>   /**
>>>> - * Detach an indirect packet mbuf -
>>>> + * Detach an indirect packet mbuf.
>>>> + *
>>>>    *  - restore original mbuf address and length values.
>>>>    *  - reset pktmbuf data and data_len to their default values.
>>>>    *  All other fields of the given packet mbuf will be left intact.
>>>> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>>>>    * @param m
>>>>    *   The indirect attached packet mbuf.
>>>>    */
>>>> -
>>>>   static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>>>>   {
>>>> -	const struct rte_mempool *mp = m->pool;
>>>> -	void *buf = RTE_MBUF_TO_BADDR(m);
>>>> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
>>>> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
>>>> -
>>>> +	struct rte_pktmbuf_pool_private *mbp_priv;
>>>> +	struct rte_mempool *mp = m->pool;
>>>> +	void *buf;
>>>> +	unsigned mhdr_size;
>>>> +
>>>> +	/* first, restore the priv_size, this is needed before calling
>>>> +	 * rte_mbuf_to_baddr() */
>>>> +	mbp_priv = rte_mempool_get_priv(mp);
>>>> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
>>>> +		mbp_priv->mbuf_data_room_size -
>>>> +		sizeof(struct rte_mbuf);
>>>
>>> I think it is better to put this priv_size calculation above into the separate function -
>>> rte_mbuf_get_priv_size(m) or something.
>>> We need it in few places, and users would probably need it anyway.
>>
>> yep, good idea
>>
>>>
>>>> +
>>>> +	buf = rte_mbuf_to_baddr(m);
>>>> +	mhdr_size = (char *)buf - (char *)m;
>>>
>>> Why do you need to recalculate mhdr_size here?
>>> As I understand it is a m->priv_size, and you just retrieved it, 2 lines above.
>>>
>>
>> It's not m->priv_size but (sizeof(rte_mbuf) + m->priv_size).
>
> Ah yes, sorry for confusion.
>
>> In both case, it requires an operation, but maybe
>>    mhdr_size = (sizeof(rte_mbuf) + m->priv_size)
>> is clearer than
>>    mhdr_size = (char *)buf - (char *)m
>>
>>
>>>> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
>>>
>>> Actually I think could just be:
>>> m->buf_physaddr = rte_mempool_virt2phy(mp, buf);
>>
>> Even if it would work, the API of rte_mempool_virt2phy()
>> says that the second argument should be "A pointer (virtual address)
>> to the element of the pool."
>> I think we should keep the initial code.
>
> Ok.
> Konstantin
>
>>
>> Regards,
>> Olivier
>>
>
Ananyev, Konstantin April 7, 2015, 5:17 p.m. UTC | #6 
Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, April 07, 2015 4:46 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> 
> Hi Konstantin,
> 
> On 04/07/2015 02:40 PM, Ananyev, Konstantin wrote:
> > Hi Olivier,
> >
> >> -----Original Message-----
> >> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> >> Sent: Monday, April 06, 2015 10:50 PM
> >> To: Ananyev, Konstantin; dev@dpdk.org
> >> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
> >> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> >>
> >> Hi Konstantin,
> >>
> >> Thanks for your comments.
> >>
> >> On 04/02/2015 07:21 PM, Ananyev, Konstantin wrote:
> >>> Hi Olivier,
> >>>
> >>>> -----Original Message-----
> >>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >>>> Sent: Tuesday, March 31, 2015 8:23 PM
> >>>> To: dev@dpdk.org
> >>>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; Olivier Matz
> >>>> Subject: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> >>>>
> >>>> From: Olivier Matz <olivier.matz@6wind.com>
> >>>>
> >>>> Add a new private_size field in mbuf structure that should
> >>>> be initialized at mbuf pool creation. This field contains the
> >>>> size of the application private data in mbufs.
> >>>>
> >>>> Introduce new static inline functions rte_mbuf_from_indirect()
> >>>> and rte_mbuf_to_baddr() to replace the existing macros, which
> >>>> take the private size in account when attaching and detaching
> >>>> mbufs.
> >>>>
> >>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >>>> ---
> >>>>   app/test-pmd/testpmd.c     |  1 +
> >>>>   examples/vhost/main.c      |  4 +--
> >>>>   lib/librte_mbuf/rte_mbuf.c |  1 +
> >>>>   lib/librte_mbuf/rte_mbuf.h | 77 +++++++++++++++++++++++++++++++++++-----------
> >>>>   4 files changed, 63 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >>>> index 3057791..c5a195a 100644
> >>>> --- a/app/test-pmd/testpmd.c
> >>>> +++ b/app/test-pmd/testpmd.c
> >>>> @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp,
> >>>>   	mb->tx_offload   = 0;
> >>>>   	mb->vlan_tci     = 0;
> >>>>   	mb->hash.rss     = 0;
> >>>> +	mb->priv_size    = 0;
> >>>>   }
> >>>>
> >>>>   static void
> >>>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> >>>> index c3fcb80..e44e82f 100644
> >>>> --- a/examples/vhost/main.c
> >>>> +++ b/examples/vhost/main.c
> >>>> @@ -139,7 +139,7 @@
> >>>>   /* Number of descriptors per cacheline. */
> >>>>   #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
> >>>>
> >>>> -#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
> >>>> +#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
> >>>>
> >>>>   /* mask of enabled ports */
> >>>>   static uint32_t enabled_port_mask = 0;
> >>>> @@ -1550,7 +1550,7 @@ attach_rxmbuf_zcp(struct virtio_net *dev)
> >>>>   static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
> >>>>   {
> >>>>   	const struct rte_mempool *mp = m->pool;
> >>>> -	void *buf = RTE_MBUF_TO_BADDR(m);
> >>>> +	void *buf = rte_mbuf_to_baddr(m);
> >>>>   	uint32_t buf_ofs;
> >>>>   	uint32_t buf_len = mp->elt_size - sizeof(*m);
> >>>>   	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
> >>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> >>>> index 526b18d..e095999 100644
> >>>> --- a/lib/librte_mbuf/rte_mbuf.c
> >>>> +++ b/lib/librte_mbuf/rte_mbuf.c
> >>>> @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
> >>>>   	m->pool = mp;
> >>>>   	m->nb_segs = 1;
> >>>>   	m->port = 0xff;
> >>>> +	m->priv_size = 0;
> >>>
> >>> Why it is 0?
> >>> Shouldn't it be the same calulations as in detach() below:
> >>> m->priv_size = /*get private size from mempool private*/;
> >>> m->buf_addr = (char *)m + sizeof(struct rte_mbuf) + m->priv_size;
> >>> m->buf_len = mp->elt_size - sizeof(struct rte_mbuf) - m->priv_size;
> >>> ?
> >>
> >> It's 0 because we also have in the function (not visible in the
> >> patch):
> >>
> >>    m->buf_addr = (char *)m + sizeof(struct rte_mbuf);
> >
> > Yep, that's why as I wrote above, I think we need to setup here all 3 fields:
> > priv_size, buf_addr, buf_len exactly in the same way as in detach().
> >
> >>
> >> It means that an application that wants to use a private area has
> >> to provide another init function derived from this default function.
> >
> > After your changes, attach/free and other functions from public mbuf API
> > rely on priv_size being set properly.
> > So I suppose 'official' pktmbuf_init() should also set it in a proper manner.
> >
> >> This was already the case before the patch series.
> >
> > Before this patch series, we don't have priv_size, so we have nothing to setup.
> >
> >>
> >> As we discussed in previous mail, I plan to propose a rework of
> >> mbuf pool initialization in another series, and my initial idea was to
> >> change this at the same time. But on the other hand it does not hurt
> >> to do this change now. I'll include it in next version.
> >
> > Ok.
> 
> Just to be sure we're on the same line:
> 
> - before the patch series
> 
>    - private area was working before that patch series if clones were not
>      used. To use a private are, the user had to provide another
>      function derived from pktmbuf_init() to change m->buf_addr and
>      m->buf_len.
>    - using both private area + clones was broken
> 
> - after the patch series
> 
>    - private area is working with or without clone. But yo use it,
>      the user still has to provide another function to change
>      m->buf_addr, m->buf_len *and m->priv_size*.
> 
> The series just fixes the fact that "clones + priv" was not working.
> It does not address the problem that providing a new pktmbuf_init()
> function is required to use privata area. To fix this, I think it
> could require a API evolution that should be part of another series.

I don't think we need new pktmbuf_init().
We just need to update it, so both pktmbuf_init() and detach() setup
buf_addr, buf_len (and priv_size) to exactly the same values.
If they don't do that, it means that you can't use attach/detach with
mempools created with pktmbuf_init() any more.

BTW, another thing that I just realised:
examples/ipv4_multicast and examples/ip_fragmentation/ -
both create a pool of mbufs with elem_size < 2K and don't populate mempool's private area -
so mbp_priv->mbuf_data_room_size == 0, for them. 

So that code in detach():

 +	mbp_priv = rte_mempool_get_priv(mp);
 +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
 +		mbp_priv->mbuf_data_room_size -
 +		sizeof(struct rte_mbuf);


Would break both these samples.
I suppose we need to handle situation when mp->elt_size < RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf),
(and probably also when mbuf_data_room_size == 0) correctly. 

Konstantin


> 
> I'll send a v4 addressing the comments soon, thanks.
> 
> Regards,
> Olivier
> 
> 
> 
> >
> >>
> >>
> >>> BTW, don't see changes in rte_pktmbuf_pool_init() to setup
> >>> mbp_priv->mbuf_data_room_size properly.
> >>> Without that changes, how can people start using that feature?
> >>> It seems that the only way now - setup priv_size and buf_len for each mbuf manually.
> >>
> >> It's the same reason than above. To use a private are, the user has
> >> to provide its own function that sets up data_room_size, derived from
> >> this pool_init default function. This was also the case before the
> >> patch series.
> >>
> >>
> >>>
> >>>>   }
> >>>>
> >>>>   /* do some sanity checks on a mbuf: panic if it fails */
> >>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >>>> index 17ba791..932fe58 100644
> >>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>> @@ -317,18 +317,51 @@ struct rte_mbuf {
> >>>>   			/* uint64_t unused:8; */
> >>>>   		};
> >>>>   	};
> >>>> +
> >>>> +	/** Size of the application private data. In case of an indirect
> >>>> +	 * mbuf, it stores the direct mbuf private data size. */
> >>>> +	uint16_t priv_size;
> >>>>   } __rte_cache_aligned;
> >>>>
> >>>>   /**
> >>>> - * Given the buf_addr returns the pointer to corresponding mbuf.
> >>>> + * Return the mbuf owning the data buffer address of an indirect mbuf.
> >>>> + *
> >>>> + * @param mi
> >>>> + *   The pointer to the indirect mbuf.
> >>>> + * @return
> >>>> + *   The address of the direct mbuf corresponding to buffer_addr.
> >>>>    */
> >>>> -#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
> >>>> +static inline struct rte_mbuf *
> >>>> +rte_mbuf_from_indirect(struct rte_mbuf *mi)
> >>>> +{
> >>>> +       struct rte_mbuf *md;
> >>>> +
> >>>> +       /* mi->buf_addr and mi->priv_size correspond to buffer and
> >>>> +	* private size of the direct mbuf */
> >>>> +       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
> >>>> +	       mi->priv_size);
> >>>
> >>> (uintptr_t)mi->buf_addr?
> >>
> >> Any clue why (uintptr_t) would be better than (char *) ?
> >
> > No big difference really, just looks a bit better to me :)
> >
> >> By the way, I added this cast because it would not compile with
> >> g++ (and probably with icc too).
> >>
> >>>
> >>>> +       return md;
> >>>> +}
> >>>>
> >>>>   /**
> >>>> - * Given the pointer to mbuf returns an address where it's  buf_addr
> >>>> - * should point to.
> >>>> + * Return the buffer address embedded in the given mbuf.
> >>>> + *
> >>>> + * The user must ensure that m->priv_size corresponds to the
> >>>> + * private size of this mbuf, which is not the case for indirect
> >>>> + * mbufs.
> >>>> + *
> >>>> + * @param md
> >>>> + *   The pointer to the mbuf.
> >>>> + * @return
> >>>> + *   The address of the data buffer owned by the mbuf.
> >>>>    */
> >>>> -#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
> >>>> +static inline char *
> >>>
> >>> Might be better to return 'void *' here.
> >>
> >> Ok, as m->buf_addr is a (void *).
> >>
> >>>
> >>>> +rte_mbuf_to_baddr(struct rte_mbuf *md)
> >>>> +{
> >>>> +       char *buffer_addr;
> >>>
> >>> uintptr_t buffer_addr?
> >>
> >> Same question than above, I don't really see why it's better than
> >> (char *).
> >>
> >>>
> >>>> +       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
> >>>> +       return buffer_addr;
> >>>> +}
> >>>>
> >>>>   /**
> >>>>    * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> >>>> @@ -688,6 +721,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >>>>
> >>>>   /**
> >>>>    * Attach packet mbuf to another packet mbuf.
> >>>> + *
> >>>>    * After attachment we refer the mbuf we attached as 'indirect',
> >>>>    * while mbuf we attached to as 'direct'.
> >>>>    * Right now, not supported:
> >>>> @@ -701,7 +735,6 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
> >>>>    * @param md
> >>>>    *   The direct packet mbuf.
> >>>>    */
> >>>> -
> >>>>   static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>>>   {
> >>>>   	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
> >>>> @@ -712,6 +745,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>>>   	mi->buf_physaddr = md->buf_physaddr;
> >>>>   	mi->buf_addr = md->buf_addr;
> >>>>   	mi->buf_len = md->buf_len;
> >>>> +	mi->priv_size = md->priv_size;
> >>>>
> >>>>   	mi->next = md->next;
> >>>>   	mi->data_off = md->data_off;
> >>>> @@ -732,7 +766,8 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>>>   }
> >>>>
> >>>>   /**
> >>>> - * Detach an indirect packet mbuf -
> >>>> + * Detach an indirect packet mbuf.
> >>>> + *
> >>>>    *  - restore original mbuf address and length values.
> >>>>    *  - reset pktmbuf data and data_len to their default values.
> >>>>    *  All other fields of the given packet mbuf will be left intact.
> >>>> @@ -740,22 +775,28 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
> >>>>    * @param m
> >>>>    *   The indirect attached packet mbuf.
> >>>>    */
> >>>> -
> >>>>   static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
> >>>>   {
> >>>> -	const struct rte_mempool *mp = m->pool;
> >>>> -	void *buf = RTE_MBUF_TO_BADDR(m);
> >>>> -	uint32_t buf_len = mp->elt_size - sizeof(*m);
> >>>> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> >>>> -
> >>>> +	struct rte_pktmbuf_pool_private *mbp_priv;
> >>>> +	struct rte_mempool *mp = m->pool;
> >>>> +	void *buf;
> >>>> +	unsigned mhdr_size;
> >>>> +
> >>>> +	/* first, restore the priv_size, this is needed before calling
> >>>> +	 * rte_mbuf_to_baddr() */
> >>>> +	mbp_priv = rte_mempool_get_priv(mp);
> >>>> +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
> >>>> +		mbp_priv->mbuf_data_room_size -
> >>>> +		sizeof(struct rte_mbuf);
> >>>
> >>> I think it is better to put this priv_size calculation above into the separate function -
> >>> rte_mbuf_get_priv_size(m) or something.
> >>> We need it in few places, and users would probably need it anyway.
> >>
> >> yep, good idea
> >>
> >>>
> >>>> +
> >>>> +	buf = rte_mbuf_to_baddr(m);
> >>>> +	mhdr_size = (char *)buf - (char *)m;
> >>>
> >>> Why do you need to recalculate mhdr_size here?
> >>> As I understand it is a m->priv_size, and you just retrieved it, 2 lines above.
> >>>
> >>
> >> It's not m->priv_size but (sizeof(rte_mbuf) + m->priv_size).
> >
> > Ah yes, sorry for confusion.
> >
> >> In both case, it requires an operation, but maybe
> >>    mhdr_size = (sizeof(rte_mbuf) + m->priv_size)
> >> is clearer than
> >>    mhdr_size = (char *)buf - (char *)m
> >>
> >>
> >>>> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
> >>>
> >>> Actually I think could just be:
> >>> m->buf_physaddr = rte_mempool_virt2phy(mp, buf);
> >>
> >> Even if it would work, the API of rte_mempool_virt2phy()
> >> says that the second argument should be "A pointer (virtual address)
> >> to the element of the pool."
> >> I think we should keep the initial code.
> >
> > Ok.
> > Konstantin
> >
> >>
> >> Regards,
> >> Olivier
> >>
> >
Olivier Matz April 8, 2015, 9:44 a.m. UTC | #7 
Hi Konstantin,

On 04/07/2015 07:17 PM, Ananyev, Konstantin wrote:
>> Just to be sure we're on the same line:
>>
>> - before the patch series
>>
>>     - private area was working before that patch series if clones were not
>>       used. To use a private are, the user had to provide another
>>       function derived from pktmbuf_init() to change m->buf_addr and
>>       m->buf_len.
>>     - using both private area + clones was broken
>>
>> - after the patch series
>>
>>     - private area is working with or without clone. But yo use it,
>>       the user still has to provide another function to change
>>       m->buf_addr, m->buf_len *and m->priv_size*.
>>
>> The series just fixes the fact that "clones + priv" was not working.
>> It does not address the problem that providing a new pktmbuf_init()
>> function is required to use privata area. To fix this, I think it
>> could require a API evolution that should be part of another series.
>
> I don't think we need new pktmbuf_init().
> We just need to update it, so both pktmbuf_init() and detach() setup
> buf_addr, buf_len (and priv_size) to exactly the same values.
> If they don't do that, it means that you can't use attach/detach with
> mempools created with pktmbuf_init() any more.
>
> BTW, another thing that I just realised:
> examples/ipv4_multicast and examples/ip_fragmentation/ -
> both create a pool of mbufs with elem_size < 2K and don't populate mempool's private area -
> so mbp_priv->mbuf_data_room_size == 0, for them.
>
> So that code in detach():
>
>   +	mbp_priv = rte_mempool_get_priv(mp);
>   +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
>   +		mbp_priv->mbuf_data_room_size -
>   +		sizeof(struct rte_mbuf);
>
>
> Would break both these samples.
> I suppose we need to handle situation when mp->elt_size < RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf),
> (and probably also when mbuf_data_room_size == 0) correctly.

Indeed. I think a mbuf pool (even with buf_len == 0 like in
ip_fragmentation example) should have a pool with a private area and
should call rte_pktmbuf_pool_init() to populate it. So
rte_pktmbuf_pool_init() has to be fixed first to use elt_size
and support the buf_len < RTE_PKTMBUF_HEADROOM, then we can
update frag/multicast examples.

Unfortunately, we don't know the size of the mbuf private area
in rte_pktmbuf_pool_init() if the opaque arg (data_room_size)
is 0, which is the default. I think it should be replaced by a structure
containing data_room_size and mbuf_priv_size, but it would break
applications that are setting data_room_size. I don't see any good
solution to do that while keeping a backward compatibility for
rte_pktmbuf_pool_init(), but as the current API is not ideal,
I think it's worth changing it and add something in the release
note.

We may also want to introduce a new helper as discussed previously:

struct rte_mempool *
rte_pktmbuf_pool_create(const char *name, unsigned n, unsigned elt_size,
	unsigned cache_size, size_t mbuf_priv_size,
	rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
	int socket_id, unsigned flags)

Any comment?


>
> Konstantin
Ananyev, Konstantin April 8, 2015, 1:45 p.m. UTC | #8 
Hi Olivier,

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, April 08, 2015 10:44 AM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
> 
> Hi Konstantin,
> 
> On 04/07/2015 07:17 PM, Ananyev, Konstantin wrote:
> >> Just to be sure we're on the same line:
> >>
> >> - before the patch series
> >>
> >>     - private area was working before that patch series if clones were not
> >>       used. To use a private are, the user had to provide another
> >>       function derived from pktmbuf_init() to change m->buf_addr and
> >>       m->buf_len.
> >>     - using both private area + clones was broken
> >>
> >> - after the patch series
> >>
> >>     - private area is working with or without clone. But yo use it,
> >>       the user still has to provide another function to change
> >>       m->buf_addr, m->buf_len *and m->priv_size*.
> >>
> >> The series just fixes the fact that "clones + priv" was not working.
> >> It does not address the problem that providing a new pktmbuf_init()
> >> function is required to use privata area. To fix this, I think it
> >> could require a API evolution that should be part of another series.
> >
> > I don't think we need new pktmbuf_init().
> > We just need to update it, so both pktmbuf_init() and detach() setup
> > buf_addr, buf_len (and priv_size) to exactly the same values.
> > If they don't do that, it means that you can't use attach/detach with
> > mempools created with pktmbuf_init() any more.
> >
> > BTW, another thing that I just realised:
> > examples/ipv4_multicast and examples/ip_fragmentation/ -
> > both create a pool of mbufs with elem_size < 2K and don't populate mempool's private area -
> > so mbp_priv->mbuf_data_room_size == 0, for them.
> >
> > So that code in detach():
> >
> >   +	mbp_priv = rte_mempool_get_priv(mp);
> >   +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
> >   +		mbp_priv->mbuf_data_room_size -
> >   +		sizeof(struct rte_mbuf);
> >
> >
> > Would break both these samples.
> > I suppose we need to handle situation when mp->elt_size < RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf),
> > (and probably also when mbuf_data_room_size == 0) correctly.
> 
> Indeed. I think a mbuf pool (even with buf_len == 0 like in
> ip_fragmentation example) should have a pool with a private area and
> should call rte_pktmbuf_pool_init() to populate it. So
> rte_pktmbuf_pool_init() has to be fixed first to use elt_size
> and support the buf_len < RTE_PKTMBUF_HEADROOM, then we can
> update frag/multicast examples.
> 
> Unfortunately, we don't know the size of the mbuf private area
> in rte_pktmbuf_pool_init() if the opaque arg (data_room_size)
> is 0, which is the default. I think it should be replaced by a structure
> containing data_room_size and mbuf_priv_size, but it would break
> applications that are setting data_room_size.

Yes, same thoughts here.

> I don't see any good
> solution to do that while keeping a backward compatibility for
> rte_pktmbuf_pool_init(), but as the current API is not ideal,
> I think it's worth changing it and add something in the release
> note.

If no one else has a better alternative than that, then I suppose it is good enough. 

> 
> We may also want to introduce a new helper as discussed previously:
> 
> struct rte_mempool *
> rte_pktmbuf_pool_create(const char *name, unsigned n, unsigned elt_size,
> 	unsigned cache_size, size_t mbuf_priv_size,
> 	rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
> 	int socket_id, unsigned flags)
> 
> Any comment?

Looks good to me.
Should we also introduce rte_pktmbuf_pool_xmem_create()? 
Konstantin

> 
> 
> >
> > Konstantin
Olivier Matz April 9, 2015, 1:06 p.m. UTC | #9 
Hi Konstantin,

On 04/08/2015 03:45 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Wednesday, April 08, 2015 10:44 AM
>> To: Ananyev, Konstantin; dev@dpdk.org
>> Cc: zoltan.kiss@linaro.org; Richardson, Bruce
>> Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data
>>
>> Hi Konstantin,
>>
>> On 04/07/2015 07:17 PM, Ananyev, Konstantin wrote:
>>>> Just to be sure we're on the same line:
>>>>
>>>> - before the patch series
>>>>
>>>>      - private area was working before that patch series if clones were not
>>>>        used. To use a private are, the user had to provide another
>>>>        function derived from pktmbuf_init() to change m->buf_addr and
>>>>        m->buf_len.
>>>>      - using both private area + clones was broken
>>>>
>>>> - after the patch series
>>>>
>>>>      - private area is working with or without clone. But yo use it,
>>>>        the user still has to provide another function to change
>>>>        m->buf_addr, m->buf_len *and m->priv_size*.
>>>>
>>>> The series just fixes the fact that "clones + priv" was not working.
>>>> It does not address the problem that providing a new pktmbuf_init()
>>>> function is required to use privata area. To fix this, I think it
>>>> could require a API evolution that should be part of another series.
>>>
>>> I don't think we need new pktmbuf_init().
>>> We just need to update it, so both pktmbuf_init() and detach() setup
>>> buf_addr, buf_len (and priv_size) to exactly the same values.
>>> If they don't do that, it means that you can't use attach/detach with
>>> mempools created with pktmbuf_init() any more.
>>>
>>> BTW, another thing that I just realised:
>>> examples/ipv4_multicast and examples/ip_fragmentation/ -
>>> both create a pool of mbufs with elem_size < 2K and don't populate mempool's private area -
>>> so mbp_priv->mbuf_data_room_size == 0, for them.
>>>
>>> So that code in detach():
>>>
>>>    +	mbp_priv = rte_mempool_get_priv(mp);
>>>    +	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
>>>    +		mbp_priv->mbuf_data_room_size -
>>>    +		sizeof(struct rte_mbuf);
>>>
>>>
>>> Would break both these samples.
>>> I suppose we need to handle situation when mp->elt_size < RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf),
>>> (and probably also when mbuf_data_room_size == 0) correctly.
>>
>> Indeed. I think a mbuf pool (even with buf_len == 0 like in
>> ip_fragmentation example) should have a pool with a private area and
>> should call rte_pktmbuf_pool_init() to populate it. So
>> rte_pktmbuf_pool_init() has to be fixed first to use elt_size
>> and support the buf_len < RTE_PKTMBUF_HEADROOM, then we can
>> update frag/multicast examples.
>>
>> Unfortunately, we don't know the size of the mbuf private area
>> in rte_pktmbuf_pool_init() if the opaque arg (data_room_size)
>> is 0, which is the default. I think it should be replaced by a structure
>> containing data_room_size and mbuf_priv_size, but it would break
>> applications that are setting data_room_size.
>
> Yes, same thoughts here.
>
>> I don't see any good
>> solution to do that while keeping a backward compatibility for
>> rte_pktmbuf_pool_init(), but as the current API is not ideal,
>> I think it's worth changing it and add something in the release
>> note.
>
> If no one else has a better alternative than that, then I suppose it is good enough.
>
>>
>> We may also want to introduce a new helper as discussed previously:
>>
>> struct rte_mempool *
>> rte_pktmbuf_pool_create(const char *name, unsigned n, unsigned elt_size,
>> 	unsigned cache_size, size_t mbuf_priv_size,
>> 	rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
>> 	int socket_id, unsigned flags)
>>
>> Any comment?
>
> Looks good to me.
> Should we also introduce rte_pktmbuf_pool_xmem_create()?

Hmmm as it's only used in one place, I'm not sure it's really
required. As the previous way to create mbuf pool using
rte_mempool_create() will still work, I think we could consider
it's a special case. If it becomes necessary, there's no problem
to add it later.

Olivier



> Konstantin
>
>>
>>
>>>
>>> Konstantin
>
Olivier Matz April 20, 2015, 3:41 p.m. UTC | #10 
The first objective of this series is to fix the support of indirect
mbufs when the application reserves a private area in mbufs. It also
removes the limitation that rte_pktmbuf_clone() is only allowed on
direct (non-cloned) mbufs. The series also contains some enhancements
and fixes in the mbuf area that makes the implementation of the
last patches easier.

Changes in v4:
- do not add a priv_size in mbuf structure, having a proper accessor
  to read it from the pool private area is clearer
- prepend some reworks in the mbuf area to simplify the implementation
  (fix mbuf initialization by not using a hardcoded mbuf size, add
  accessors for mbuf pool private area, add a helper to create a
  mbuf pool)

Changes in v3:
- a mbuf can now attach to another one that have a different private
  size. In this case, the m->priv_size corresponds to the size of the
  private area of the direct mbuf.
- add comments to reflect these changes
- minor style modifications

Changes in v2:
- do not change the use of MBUF_EXT_MEM() in vhost
- change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
  one parameter
- fix and rework rte_pktmbuf_detach()
- move m->priv_size in second mbuf cache line
- fix mbuf free in test error case


Olivier Matz (12):
  mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
  examples: always initialize mbuf_pool private area
  mbuf: add accessors to get data room size and private size
  mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
  testpmd: use standard functions to initialize mbufs and mbuf pool
  mbuf: introduce a new helper to create a mbuf pool
  apps: use rte_pktmbuf_pool_create to create mbuf pools
  mbuf: fix clone support when application uses private mbuf data
  mbuf: allow to clone an indirect mbuf
  test/mbuf: rename mc variable in m
  test/mbuf: enhance mbuf refcnt test
  test/mbuf: verify that cloning a clone works properly

 app/test-pipeline/init.c                           |  15 +-
 app/test-pmd/testpmd.c                             |  78 +--------
 app/test/test_distributor.c                        |  10 +-
 app/test/test_distributor_perf.c                   |  10 +-
 app/test/test_kni.c                                |  16 +-
 app/test/test_link_bonding.c                       |  10 +-
 app/test/test_link_bonding_mode4.c                 |  12 +-
 app/test/test_mbuf.c                               | 110 +++++++++---
 app/test/test_pmd_perf.c                           |  11 +-
 app/test/test_pmd_ring.c                           |  10 +-
 app/test/test_reorder.c                            |  10 +-
 app/test/test_sched.c                              |  16 +-
 app/test/test_table.c                              |   9 +-
 app/test/test_table.h                              |   3 +-
 doc/guides/rel_notes/updating_apps.rst             |  16 ++
 examples/bond/main.c                               |  10 +-
 examples/distributor/main.c                        |  11 +-
 examples/dpdk_qat/main.c                           |  10 +-
 examples/exception_path/main.c                     |  14 +-
 examples/ip_fragmentation/main.c                   |  18 +-
 examples/ip_pipeline/init.c                        |  28 +--
 examples/ipv4_multicast/main.c                     |  21 +--
 examples/kni/main.c                                |  12 +-
 examples/l2fwd-ivshmem/host/host.c                 |  10 +-
 examples/l2fwd-jobstats/main.c                     |  10 +-
 examples/l2fwd/main.c                              |  11 +-
 examples/l3fwd-acl/main.c                          |  11 +-
 examples/l3fwd-power/main.c                        |  11 +-
 examples/l3fwd-vf/main.c                           |  12 +-
 examples/l3fwd/main.c                              |  10 +-
 examples/link_status_interrupt/main.c              |  10 +-
 examples/load_balancer/init.c                      |  12 +-
 examples/load_balancer/main.h                      |   4 +-
 .../client_server_mp/mp_server/init.c              |  10 +-
 examples/multi_process/symmetric_mp/main.c         |  10 +-
 examples/netmap_compat/bridge/bridge.c             |  12 +-
 examples/packet_ordering/main.c                    |  11 +-
 examples/qos_meter/main.c                          |   7 +-
 examples/qos_sched/init.c                          |  10 +-
 examples/qos_sched/main.h                          |   2 +-
 examples/quota_watermark/include/conf.h            |   2 +-
 examples/quota_watermark/qw/main.c                 |   7 +-
 examples/rxtx_callbacks/main.c                     |  11 +-
 examples/skeleton/basicfwd.c                       |  13 +-
 examples/vhost/main.c                              |  31 ++--
 examples/vhost_xen/main.c                          |  11 +-
 examples/vmdq/main.c                               |  11 +-
 examples/vmdq_dcb/main.c                           |  10 +-
 lib/librte_ether/rte_ethdev.c                      |   4 +-
 lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
 lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
 lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
 lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
 lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
 lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
 lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
 lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
 lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
 lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
 lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
 61 files changed, 499 insertions(+), 566 deletions(-)
Neil Horman April 20, 2015, 4:53 p.m. UTC | #11 
On Mon, Apr 20, 2015 at 05:41:24PM +0200, Olivier Matz wrote:
> The first objective of this series is to fix the support of indirect
> mbufs when the application reserves a private area in mbufs. It also
> removes the limitation that rte_pktmbuf_clone() is only allowed on
> direct (non-cloned) mbufs. The series also contains some enhancements
> and fixes in the mbuf area that makes the implementation of the
> last patches easier.
> 
> Changes in v4:
> - do not add a priv_size in mbuf structure, having a proper accessor
>   to read it from the pool private area is clearer
> - prepend some reworks in the mbuf area to simplify the implementation
>   (fix mbuf initialization by not using a hardcoded mbuf size, add
>   accessors for mbuf pool private area, add a helper to create a
>   mbuf pool)
> 
> Changes in v3:
> - a mbuf can now attach to another one that have a different private
>   size. In this case, the m->priv_size corresponds to the size of the
>   private area of the direct mbuf.
> - add comments to reflect these changes
> - minor style modifications
> 
> Changes in v2:
> - do not change the use of MBUF_EXT_MEM() in vhost
> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
>   one parameter
> - fix and rework rte_pktmbuf_detach()
> - move m->priv_size in second mbuf cache line
> - fix mbuf free in test error case
> 
> 
> Olivier Matz (12):
>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>   examples: always initialize mbuf_pool private area
>   mbuf: add accessors to get data room size and private size
>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>   testpmd: use standard functions to initialize mbufs and mbuf pool
>   mbuf: introduce a new helper to create a mbuf pool
>   apps: use rte_pktmbuf_pool_create to create mbuf pools
>   mbuf: fix clone support when application uses private mbuf data
>   mbuf: allow to clone an indirect mbuf
>   test/mbuf: rename mc variable in m
>   test/mbuf: enhance mbuf refcnt test
>   test/mbuf: verify that cloning a clone works properly
> 
>  app/test-pipeline/init.c                           |  15 +-
>  app/test-pmd/testpmd.c                             |  78 +--------
>  app/test/test_distributor.c                        |  10 +-
>  app/test/test_distributor_perf.c                   |  10 +-
>  app/test/test_kni.c                                |  16 +-
>  app/test/test_link_bonding.c                       |  10 +-
>  app/test/test_link_bonding_mode4.c                 |  12 +-
>  app/test/test_mbuf.c                               | 110 +++++++++---
>  app/test/test_pmd_perf.c                           |  11 +-
>  app/test/test_pmd_ring.c                           |  10 +-
>  app/test/test_reorder.c                            |  10 +-
>  app/test/test_sched.c                              |  16 +-
>  app/test/test_table.c                              |   9 +-
>  app/test/test_table.h                              |   3 +-
>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
>  examples/bond/main.c                               |  10 +-
>  examples/distributor/main.c                        |  11 +-
>  examples/dpdk_qat/main.c                           |  10 +-
>  examples/exception_path/main.c                     |  14 +-
>  examples/ip_fragmentation/main.c                   |  18 +-
>  examples/ip_pipeline/init.c                        |  28 +--
>  examples/ipv4_multicast/main.c                     |  21 +--
>  examples/kni/main.c                                |  12 +-
>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
>  examples/l2fwd-jobstats/main.c                     |  10 +-
>  examples/l2fwd/main.c                              |  11 +-
>  examples/l3fwd-acl/main.c                          |  11 +-
>  examples/l3fwd-power/main.c                        |  11 +-
>  examples/l3fwd-vf/main.c                           |  12 +-
>  examples/l3fwd/main.c                              |  10 +-
>  examples/link_status_interrupt/main.c              |  10 +-
>  examples/load_balancer/init.c                      |  12 +-
>  examples/load_balancer/main.h                      |   4 +-
>  .../client_server_mp/mp_server/init.c              |  10 +-
>  examples/multi_process/symmetric_mp/main.c         |  10 +-
>  examples/netmap_compat/bridge/bridge.c             |  12 +-
>  examples/packet_ordering/main.c                    |  11 +-
>  examples/qos_meter/main.c                          |   7 +-
>  examples/qos_sched/init.c                          |  10 +-
>  examples/qos_sched/main.h                          |   2 +-
>  examples/quota_watermark/include/conf.h            |   2 +-
>  examples/quota_watermark/qw/main.c                 |   7 +-
>  examples/rxtx_callbacks/main.c                     |  11 +-
>  examples/skeleton/basicfwd.c                       |  13 +-
>  examples/vhost/main.c                              |  31 ++--
>  examples/vhost_xen/main.c                          |  11 +-
>  examples/vmdq/main.c                               |  11 +-
>  examples/vmdq_dcb/main.c                           |  10 +-
>  lib/librte_ether/rte_ethdev.c                      |   4 +-
>  lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
>  lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
>  61 files changed, 499 insertions(+), 566 deletions(-)
> 
> -- 
> 2.1.4
> 
> 


[nhorman@hmsreliant dpdk]$ make config T=x86_64-native-linuxapp-gcc
Configuration done
[nhorman@hmsreliant dpdk]$ make
...
 CC test_kvargs.o
  LD test
test_pmd_perf.o: In function `test_pmd_perf':
test_pmd_perf.c:(.text+0xd1b): undefined reference to `rte_pktmbuf_pool_create'
test_table.o: In function `test_table':
test_table.c:(.text+0x2da): undefined reference to `rte_pktmbuf_pool_create'
test_mbuf.o: In function `test_mbuf':
test_mbuf.c:(.text+0x3d5c): undefined reference to `rte_pktmbuf_pool_create'
test_mbuf.c:(.text+0x542a): undefined reference to `rte_pktmbuf_pool_create'
test_sched.o: In function `test_sched':
test_sched.c:(.text+0x5b1): undefined reference to `rte_pktmbuf_pool_create'
test_distributor.o:test_distributor.c:(.text+0x40a9): more undefined references
to `rte_pktmbuf_pool_create' follow

Neil
Olivier Matz April 20, 2015, 5:07 p.m. UTC | #12 
Hi Neil,

On 04/20/2015 06:53 PM, Neil Horman wrote:
> On Mon, Apr 20, 2015 at 05:41:24PM +0200, Olivier Matz wrote:
>> The first objective of this series is to fix the support of indirect
>> mbufs when the application reserves a private area in mbufs. It also
>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>> direct (non-cloned) mbufs. The series also contains some enhancements
>> and fixes in the mbuf area that makes the implementation of the
>> last patches easier.
>>
>> Changes in v4:
>> - do not add a priv_size in mbuf structure, having a proper accessor
>>   to read it from the pool private area is clearer
>> - prepend some reworks in the mbuf area to simplify the implementation
>>   (fix mbuf initialization by not using a hardcoded mbuf size, add
>>   accessors for mbuf pool private area, add a helper to create a
>>   mbuf pool)
>>
>> Changes in v3:
>> - a mbuf can now attach to another one that have a different private
>>   size. In this case, the m->priv_size corresponds to the size of the
>>   private area of the direct mbuf.
>> - add comments to reflect these changes
>> - minor style modifications
>>
>> Changes in v2:
>> - do not change the use of MBUF_EXT_MEM() in vhost
>> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
>>   one parameter
>> - fix and rework rte_pktmbuf_detach()
>> - move m->priv_size in second mbuf cache line
>> - fix mbuf free in test error case
>>
>>
>> Olivier Matz (12):
>>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>>   examples: always initialize mbuf_pool private area
>>   mbuf: add accessors to get data room size and private size
>>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>>   testpmd: use standard functions to initialize mbufs and mbuf pool
>>   mbuf: introduce a new helper to create a mbuf pool
>>   apps: use rte_pktmbuf_pool_create to create mbuf pools
>>   mbuf: fix clone support when application uses private mbuf data
>>   mbuf: allow to clone an indirect mbuf
>>   test/mbuf: rename mc variable in m
>>   test/mbuf: enhance mbuf refcnt test
>>   test/mbuf: verify that cloning a clone works properly
>>
>>  app/test-pipeline/init.c                           |  15 +-
>>  app/test-pmd/testpmd.c                             |  78 +--------
>>  app/test/test_distributor.c                        |  10 +-
>>  app/test/test_distributor_perf.c                   |  10 +-
>>  app/test/test_kni.c                                |  16 +-
>>  app/test/test_link_bonding.c                       |  10 +-
>>  app/test/test_link_bonding_mode4.c                 |  12 +-
>>  app/test/test_mbuf.c                               | 110 +++++++++---
>>  app/test/test_pmd_perf.c                           |  11 +-
>>  app/test/test_pmd_ring.c                           |  10 +-
>>  app/test/test_reorder.c                            |  10 +-
>>  app/test/test_sched.c                              |  16 +-
>>  app/test/test_table.c                              |   9 +-
>>  app/test/test_table.h                              |   3 +-
>>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
>>  examples/bond/main.c                               |  10 +-
>>  examples/distributor/main.c                        |  11 +-
>>  examples/dpdk_qat/main.c                           |  10 +-
>>  examples/exception_path/main.c                     |  14 +-
>>  examples/ip_fragmentation/main.c                   |  18 +-
>>  examples/ip_pipeline/init.c                        |  28 +--
>>  examples/ipv4_multicast/main.c                     |  21 +--
>>  examples/kni/main.c                                |  12 +-
>>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
>>  examples/l2fwd-jobstats/main.c                     |  10 +-
>>  examples/l2fwd/main.c                              |  11 +-
>>  examples/l3fwd-acl/main.c                          |  11 +-
>>  examples/l3fwd-power/main.c                        |  11 +-
>>  examples/l3fwd-vf/main.c                           |  12 +-
>>  examples/l3fwd/main.c                              |  10 +-
>>  examples/link_status_interrupt/main.c              |  10 +-
>>  examples/load_balancer/init.c                      |  12 +-
>>  examples/load_balancer/main.h                      |   4 +-
>>  .../client_server_mp/mp_server/init.c              |  10 +-
>>  examples/multi_process/symmetric_mp/main.c         |  10 +-
>>  examples/netmap_compat/bridge/bridge.c             |  12 +-
>>  examples/packet_ordering/main.c                    |  11 +-
>>  examples/qos_meter/main.c                          |   7 +-
>>  examples/qos_sched/init.c                          |  10 +-
>>  examples/qos_sched/main.h                          |   2 +-
>>  examples/quota_watermark/include/conf.h            |   2 +-
>>  examples/quota_watermark/qw/main.c                 |   7 +-
>>  examples/rxtx_callbacks/main.c                     |  11 +-
>>  examples/skeleton/basicfwd.c                       |  13 +-
>>  examples/vhost/main.c                              |  31 ++--
>>  examples/vhost_xen/main.c                          |  11 +-
>>  examples/vmdq/main.c                               |  11 +-
>>  examples/vmdq_dcb/main.c                           |  10 +-
>>  lib/librte_ether/rte_ethdev.c                      |   4 +-
>>  lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
>>  lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
>>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
>>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
>>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
>>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
>>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
>>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
>>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
>>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
>>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
>>  61 files changed, 499 insertions(+), 566 deletions(-)
>>
>> -- 
>> 2.1.4
>>
>>
> 
> 
> [nhorman@hmsreliant dpdk]$ make config T=x86_64-native-linuxapp-gcc
> Configuration done
> [nhorman@hmsreliant dpdk]$ make
> ...
>  CC test_kvargs.o
>   LD test
> test_pmd_perf.o: In function `test_pmd_perf':
> test_pmd_perf.c:(.text+0xd1b): undefined reference to `rte_pktmbuf_pool_create'
> test_table.o: In function `test_table':
> test_table.c:(.text+0x2da): undefined reference to `rte_pktmbuf_pool_create'
> test_mbuf.o: In function `test_mbuf':
> test_mbuf.c:(.text+0x3d5c): undefined reference to `rte_pktmbuf_pool_create'
> test_mbuf.c:(.text+0x542a): undefined reference to `rte_pktmbuf_pool_create'
> test_sched.o: In function `test_sched':
> test_sched.c:(.text+0x5b1): undefined reference to `rte_pktmbuf_pool_create'
> test_distributor.o:test_distributor.c:(.text+0x40a9): more undefined references
> to `rte_pktmbuf_pool_create' follow

I cannot reproduce the issue. Maybe you forgot to clean something?


matz@glumotte:~/DEV/toto$ git clone http://dpdk.org/git/dpdk
Cloning into 'dpdk'...
remote: Counting objects: 24195, done.
remote: Compressing objects: 100% (5000/5000), done.
remote: Total 24195 (delta 19183), reused 23953 (delta 19014)
Receiving objects: 100% (24195/24195), 20.39 MiB | 199.00 KiB/s, done.
Resolving deltas: 100% (19183/19183), done.
Checking connectivity... done.
matz@glumotte:~/DEV/toto$ cd dpdk/
matz@glumotte:~/DEV/toto/dpdk$ git am /tmp/mbuf_clone_v4/*
Applying: mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
Applying: examples: always initialize mbuf_pool private area
Applying: mbuf: add accessors to get data room size and private size
Applying: mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
Applying: testpmd: use standard functions to initialize mbufs and mbuf pool
Applying: mbuf: introduce a new helper to create a mbuf pool
Applying: apps: use rte_pktmbuf_pool_create to create mbuf pools
Applying: mbuf: fix clone support when application uses private mbuf data
Applying: mbuf: allow to clone an indirect mbuf
Applying: test/mbuf: rename mc variable in m
Applying: test/mbuf: enhance mbuf refcnt test
Applying: test/mbuf: verify that cloning a clone works properly
matz@glumotte:~/DEV/toto/dpdk$ make config T=x86_64-native-linuxapp-gcc
Configuration done
matz@glumotte:~/DEV/toto/dpdk$ make
== Build lib
[...]
  CC main.o
  LD dump_cfg
  INSTALL-APP dump_cfg
  INSTALL-MAP dump_cfg.map
Build complete


Regards,
Olivier
Neil Horman April 20, 2015, 5:21 p.m. UTC | #13 
On Mon, Apr 20, 2015 at 07:07:31PM +0200, Olivier MATZ wrote:
> Hi Neil,
> 
> On 04/20/2015 06:53 PM, Neil Horman wrote:
> > On Mon, Apr 20, 2015 at 05:41:24PM +0200, Olivier Matz wrote:
> >> The first objective of this series is to fix the support of indirect
> >> mbufs when the application reserves a private area in mbufs. It also
> >> removes the limitation that rte_pktmbuf_clone() is only allowed on
> >> direct (non-cloned) mbufs. The series also contains some enhancements
> >> and fixes in the mbuf area that makes the implementation of the
> >> last patches easier.
> >>
> >> Changes in v4:
> >> - do not add a priv_size in mbuf structure, having a proper accessor
> >>   to read it from the pool private area is clearer
> >> - prepend some reworks in the mbuf area to simplify the implementation
> >>   (fix mbuf initialization by not using a hardcoded mbuf size, add
> >>   accessors for mbuf pool private area, add a helper to create a
> >>   mbuf pool)
> >>
> >> Changes in v3:
> >> - a mbuf can now attach to another one that have a different private
> >>   size. In this case, the m->priv_size corresponds to the size of the
> >>   private area of the direct mbuf.
> >> - add comments to reflect these changes
> >> - minor style modifications
> >>
> >> Changes in v2:
> >> - do not change the use of MBUF_EXT_MEM() in vhost
> >> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
> >>   one parameter
> >> - fix and rework rte_pktmbuf_detach()
> >> - move m->priv_size in second mbuf cache line
> >> - fix mbuf free in test error case
> >>
> >>
> >> Olivier Matz (12):
> >>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
> >>   examples: always initialize mbuf_pool private area
> >>   mbuf: add accessors to get data room size and private size
> >>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
> >>   testpmd: use standard functions to initialize mbufs and mbuf pool
> >>   mbuf: introduce a new helper to create a mbuf pool
> >>   apps: use rte_pktmbuf_pool_create to create mbuf pools
> >>   mbuf: fix clone support when application uses private mbuf data
> >>   mbuf: allow to clone an indirect mbuf
> >>   test/mbuf: rename mc variable in m
> >>   test/mbuf: enhance mbuf refcnt test
> >>   test/mbuf: verify that cloning a clone works properly
> >>
> >>  app/test-pipeline/init.c                           |  15 +-
> >>  app/test-pmd/testpmd.c                             |  78 +--------
> >>  app/test/test_distributor.c                        |  10 +-
> >>  app/test/test_distributor_perf.c                   |  10 +-
> >>  app/test/test_kni.c                                |  16 +-
> >>  app/test/test_link_bonding.c                       |  10 +-
> >>  app/test/test_link_bonding_mode4.c                 |  12 +-
> >>  app/test/test_mbuf.c                               | 110 +++++++++---
> >>  app/test/test_pmd_perf.c                           |  11 +-
> >>  app/test/test_pmd_ring.c                           |  10 +-
> >>  app/test/test_reorder.c                            |  10 +-
> >>  app/test/test_sched.c                              |  16 +-
> >>  app/test/test_table.c                              |   9 +-
> >>  app/test/test_table.h                              |   3 +-
> >>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
> >>  examples/bond/main.c                               |  10 +-
> >>  examples/distributor/main.c                        |  11 +-
> >>  examples/dpdk_qat/main.c                           |  10 +-
> >>  examples/exception_path/main.c                     |  14 +-
> >>  examples/ip_fragmentation/main.c                   |  18 +-
> >>  examples/ip_pipeline/init.c                        |  28 +--
> >>  examples/ipv4_multicast/main.c                     |  21 +--
> >>  examples/kni/main.c                                |  12 +-
> >>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
> >>  examples/l2fwd-jobstats/main.c                     |  10 +-
> >>  examples/l2fwd/main.c                              |  11 +-
> >>  examples/l3fwd-acl/main.c                          |  11 +-
> >>  examples/l3fwd-power/main.c                        |  11 +-
> >>  examples/l3fwd-vf/main.c                           |  12 +-
> >>  examples/l3fwd/main.c                              |  10 +-
> >>  examples/link_status_interrupt/main.c              |  10 +-
> >>  examples/load_balancer/init.c                      |  12 +-
> >>  examples/load_balancer/main.h                      |   4 +-
> >>  .../client_server_mp/mp_server/init.c              |  10 +-
> >>  examples/multi_process/symmetric_mp/main.c         |  10 +-
> >>  examples/netmap_compat/bridge/bridge.c             |  12 +-
> >>  examples/packet_ordering/main.c                    |  11 +-
> >>  examples/qos_meter/main.c                          |   7 +-
> >>  examples/qos_sched/init.c                          |  10 +-
> >>  examples/qos_sched/main.h                          |   2 +-
> >>  examples/quota_watermark/include/conf.h            |   2 +-
> >>  examples/quota_watermark/qw/main.c                 |   7 +-
> >>  examples/rxtx_callbacks/main.c                     |  11 +-
> >>  examples/skeleton/basicfwd.c                       |  13 +-
> >>  examples/vhost/main.c                              |  31 ++--
> >>  examples/vhost_xen/main.c                          |  11 +-
> >>  examples/vmdq/main.c                               |  11 +-
> >>  examples/vmdq_dcb/main.c                           |  10 +-
> >>  lib/librte_ether/rte_ethdev.c                      |   4 +-
> >>  lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
> >>  lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
> >>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
> >>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
> >>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
> >>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
> >>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
> >>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
> >>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
> >>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
> >>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
> >>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
> >>  61 files changed, 499 insertions(+), 566 deletions(-)
> >>
> >> -- 
> >> 2.1.4
> >>
> >>
> > 
> > 
> > [nhorman@hmsreliant dpdk]$ make config T=x86_64-native-linuxapp-gcc
> > Configuration done
> > [nhorman@hmsreliant dpdk]$ make
> > ...
> >  CC test_kvargs.o
> >   LD test
> > test_pmd_perf.o: In function `test_pmd_perf':
> > test_pmd_perf.c:(.text+0xd1b): undefined reference to `rte_pktmbuf_pool_create'
> > test_table.o: In function `test_table':
> > test_table.c:(.text+0x2da): undefined reference to `rte_pktmbuf_pool_create'
> > test_mbuf.o: In function `test_mbuf':
> > test_mbuf.c:(.text+0x3d5c): undefined reference to `rte_pktmbuf_pool_create'
> > test_mbuf.c:(.text+0x542a): undefined reference to `rte_pktmbuf_pool_create'
> > test_sched.o: In function `test_sched':
> > test_sched.c:(.text+0x5b1): undefined reference to `rte_pktmbuf_pool_create'
> > test_distributor.o:test_distributor.c:(.text+0x40a9): more undefined references
> > to `rte_pktmbuf_pool_create' follow
> 
> I cannot reproduce the issue. Maybe you forgot to clean something?
> 
Nope, fresh clone.  You didn't try building with shared libraries configured :)
Neil

> 
> matz@glumotte:~/DEV/toto$ git clone http://dpdk.org/git/dpdk
> Cloning into 'dpdk'...
> remote: Counting objects: 24195, done.
> remote: Compressing objects: 100% (5000/5000), done.
> remote: Total 24195 (delta 19183), reused 23953 (delta 19014)
> Receiving objects: 100% (24195/24195), 20.39 MiB | 199.00 KiB/s, done.
> Resolving deltas: 100% (19183/19183), done.
> Checking connectivity... done.
> matz@glumotte:~/DEV/toto$ cd dpdk/
> matz@glumotte:~/DEV/toto/dpdk$ git am /tmp/mbuf_clone_v4/*
> Applying: mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
> Applying: examples: always initialize mbuf_pool private area
> Applying: mbuf: add accessors to get data room size and private size
> Applying: mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
> Applying: testpmd: use standard functions to initialize mbufs and mbuf pool
> Applying: mbuf: introduce a new helper to create a mbuf pool
> Applying: apps: use rte_pktmbuf_pool_create to create mbuf pools
> Applying: mbuf: fix clone support when application uses private mbuf data
> Applying: mbuf: allow to clone an indirect mbuf
> Applying: test/mbuf: rename mc variable in m
> Applying: test/mbuf: enhance mbuf refcnt test
> Applying: test/mbuf: verify that cloning a clone works properly
> matz@glumotte:~/DEV/toto/dpdk$ make config T=x86_64-native-linuxapp-gcc
> Configuration done
> matz@glumotte:~/DEV/toto/dpdk$ make
> == Build lib
> [...]
>   CC main.o
>   LD dump_cfg
>   INSTALL-APP dump_cfg
>   INSTALL-MAP dump_cfg.map
> Build complete
> 
> 
> Regards,
> Olivier
>
Olivier Matz April 20, 2015, 6:24 p.m. UTC | #14 
Hi Neil,

On 04/20/2015 07:21 PM, Neil Horman wrote:
> On Mon, Apr 20, 2015 at 07:07:31PM +0200, Olivier MATZ wrote:
>> Hi Neil,
>>
>> On 04/20/2015 06:53 PM, Neil Horman wrote:
>>> On Mon, Apr 20, 2015 at 05:41:24PM +0200, Olivier Matz wrote:
>>>> The first objective of this series is to fix the support of indirect
>>>> mbufs when the application reserves a private area in mbufs. It also
>>>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>>>> direct (non-cloned) mbufs. The series also contains some enhancements
>>>> and fixes in the mbuf area that makes the implementation of the
>>>> last patches easier.
>>>>
>>>> Changes in v4:
>>>> - do not add a priv_size in mbuf structure, having a proper accessor
>>>>   to read it from the pool private area is clearer
>>>> - prepend some reworks in the mbuf area to simplify the implementation
>>>>   (fix mbuf initialization by not using a hardcoded mbuf size, add
>>>>   accessors for mbuf pool private area, add a helper to create a
>>>>   mbuf pool)
>>>>
>>>> Changes in v3:
>>>> - a mbuf can now attach to another one that have a different private
>>>>   size. In this case, the m->priv_size corresponds to the size of the
>>>>   private area of the direct mbuf.
>>>> - add comments to reflect these changes
>>>> - minor style modifications
>>>>
>>>> Changes in v2:
>>>> - do not change the use of MBUF_EXT_MEM() in vhost
>>>> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
>>>>   one parameter
>>>> - fix and rework rte_pktmbuf_detach()
>>>> - move m->priv_size in second mbuf cache line
>>>> - fix mbuf free in test error case
>>>>
>>>>
>>>> Olivier Matz (12):
>>>>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>>>>   examples: always initialize mbuf_pool private area
>>>>   mbuf: add accessors to get data room size and private size
>>>>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>>>>   testpmd: use standard functions to initialize mbufs and mbuf pool
>>>>   mbuf: introduce a new helper to create a mbuf pool
>>>>   apps: use rte_pktmbuf_pool_create to create mbuf pools
>>>>   mbuf: fix clone support when application uses private mbuf data
>>>>   mbuf: allow to clone an indirect mbuf
>>>>   test/mbuf: rename mc variable in m
>>>>   test/mbuf: enhance mbuf refcnt test
>>>>   test/mbuf: verify that cloning a clone works properly
>>>>
>>>>  app/test-pipeline/init.c                           |  15 +-
>>>>  app/test-pmd/testpmd.c                             |  78 +--------
>>>>  app/test/test_distributor.c                        |  10 +-
>>>>  app/test/test_distributor_perf.c                   |  10 +-
>>>>  app/test/test_kni.c                                |  16 +-
>>>>  app/test/test_link_bonding.c                       |  10 +-
>>>>  app/test/test_link_bonding_mode4.c                 |  12 +-
>>>>  app/test/test_mbuf.c                               | 110 +++++++++---
>>>>  app/test/test_pmd_perf.c                           |  11 +-
>>>>  app/test/test_pmd_ring.c                           |  10 +-
>>>>  app/test/test_reorder.c                            |  10 +-
>>>>  app/test/test_sched.c                              |  16 +-
>>>>  app/test/test_table.c                              |   9 +-
>>>>  app/test/test_table.h                              |   3 +-
>>>>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
>>>>  examples/bond/main.c                               |  10 +-
>>>>  examples/distributor/main.c                        |  11 +-
>>>>  examples/dpdk_qat/main.c                           |  10 +-
>>>>  examples/exception_path/main.c                     |  14 +-
>>>>  examples/ip_fragmentation/main.c                   |  18 +-
>>>>  examples/ip_pipeline/init.c                        |  28 +--
>>>>  examples/ipv4_multicast/main.c                     |  21 +--
>>>>  examples/kni/main.c                                |  12 +-
>>>>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
>>>>  examples/l2fwd-jobstats/main.c                     |  10 +-
>>>>  examples/l2fwd/main.c                              |  11 +-
>>>>  examples/l3fwd-acl/main.c                          |  11 +-
>>>>  examples/l3fwd-power/main.c                        |  11 +-
>>>>  examples/l3fwd-vf/main.c                           |  12 +-
>>>>  examples/l3fwd/main.c                              |  10 +-
>>>>  examples/link_status_interrupt/main.c              |  10 +-
>>>>  examples/load_balancer/init.c                      |  12 +-
>>>>  examples/load_balancer/main.h                      |   4 +-
>>>>  .../client_server_mp/mp_server/init.c              |  10 +-
>>>>  examples/multi_process/symmetric_mp/main.c         |  10 +-
>>>>  examples/netmap_compat/bridge/bridge.c             |  12 +-
>>>>  examples/packet_ordering/main.c                    |  11 +-
>>>>  examples/qos_meter/main.c                          |   7 +-
>>>>  examples/qos_sched/init.c                          |  10 +-
>>>>  examples/qos_sched/main.h                          |   2 +-
>>>>  examples/quota_watermark/include/conf.h            |   2 +-
>>>>  examples/quota_watermark/qw/main.c                 |   7 +-
>>>>  examples/rxtx_callbacks/main.c                     |  11 +-
>>>>  examples/skeleton/basicfwd.c                       |  13 +-
>>>>  examples/vhost/main.c                              |  31 ++--
>>>>  examples/vhost_xen/main.c                          |  11 +-
>>>>  examples/vmdq/main.c                               |  11 +-
>>>>  examples/vmdq_dcb/main.c                           |  10 +-
>>>>  lib/librte_ether/rte_ethdev.c                      |   4 +-
>>>>  lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
>>>>  lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
>>>>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
>>>>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
>>>>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
>>>>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
>>>>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
>>>>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
>>>>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
>>>>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
>>>>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
>>>>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
>>>>  61 files changed, 499 insertions(+), 566 deletions(-)
>>>>
>>>> -- 
>>>> 2.1.4
>>>>
>>>>
>>>
>>>
>>> [nhorman@hmsreliant dpdk]$ make config T=x86_64-native-linuxapp-gcc
>>> Configuration done
>>> [nhorman@hmsreliant dpdk]$ make
>>> ...
>>>  CC test_kvargs.o
>>>   LD test
>>> test_pmd_perf.o: In function `test_pmd_perf':
>>> test_pmd_perf.c:(.text+0xd1b): undefined reference to `rte_pktmbuf_pool_create'
>>> test_table.o: In function `test_table':
>>> test_table.c:(.text+0x2da): undefined reference to `rte_pktmbuf_pool_create'
>>> test_mbuf.o: In function `test_mbuf':
>>> test_mbuf.c:(.text+0x3d5c): undefined reference to `rte_pktmbuf_pool_create'
>>> test_mbuf.c:(.text+0x542a): undefined reference to `rte_pktmbuf_pool_create'
>>> test_sched.o: In function `test_sched':
>>> test_sched.c:(.text+0x5b1): undefined reference to `rte_pktmbuf_pool_create'
>>> test_distributor.o:test_distributor.c:(.text+0x40a9): more undefined references
>>> to `rte_pktmbuf_pool_create' follow
>>
>> I cannot reproduce the issue. Maybe you forgot to clean something?
>>
> Nope, fresh clone.  You didn't try building with shared libraries configured :)
> Neil

OK, thanks for reporting. I'll fix that and add it to my checklist
for next times.

Regards,
Olivier


> 
>>
>> matz@glumotte:~/DEV/toto$ git clone http://dpdk.org/git/dpdk
>> Cloning into 'dpdk'...
>> remote: Counting objects: 24195, done.
>> remote: Compressing objects: 100% (5000/5000), done.
>> remote: Total 24195 (delta 19183), reused 23953 (delta 19014)
>> Receiving objects: 100% (24195/24195), 20.39 MiB | 199.00 KiB/s, done.
>> Resolving deltas: 100% (19183/19183), done.
>> Checking connectivity... done.
>> matz@glumotte:~/DEV/toto$ cd dpdk/
>> matz@glumotte:~/DEV/toto/dpdk$ git am /tmp/mbuf_clone_v4/*
>> Applying: mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>> Applying: examples: always initialize mbuf_pool private area
>> Applying: mbuf: add accessors to get data room size and private size
>> Applying: mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>> Applying: testpmd: use standard functions to initialize mbufs and mbuf pool
>> Applying: mbuf: introduce a new helper to create a mbuf pool
>> Applying: apps: use rte_pktmbuf_pool_create to create mbuf pools
>> Applying: mbuf: fix clone support when application uses private mbuf data
>> Applying: mbuf: allow to clone an indirect mbuf
>> Applying: test/mbuf: rename mc variable in m
>> Applying: test/mbuf: enhance mbuf refcnt test
>> Applying: test/mbuf: verify that cloning a clone works properly
>> matz@glumotte:~/DEV/toto/dpdk$ make config T=x86_64-native-linuxapp-gcc
>> Configuration done
>> matz@glumotte:~/DEV/toto/dpdk$ make
>> == Build lib
>> [...]
>>   CC main.o
>>   LD dump_cfg
>>   INSTALL-APP dump_cfg
>>   INSTALL-MAP dump_cfg.map
>> Build complete
>>
>>
>> Regards,
>> Olivier
>>
Olivier Matz April 21, 2015, 9:55 a.m. UTC | #15 
The first objective of this series is to fix the support of indirect
mbufs when the application reserves a private area in mbufs. It also
removes the limitation that rte_pktmbuf_clone() is only allowed on
direct (non-cloned) mbufs. The series also contains some enhancements
and fixes in the mbuf area that makes the implementation of the
last patches easier.

Changes in v5:
- update rte_mbuf_version.map to fix compilation with shared libraries

Changes in v4:
- do not add a priv_size in mbuf structure, having a proper accessor
  to read it from the pool private area is clearer
- prepend some reworks in the mbuf area to simplify the implementation
  (fix mbuf initialization by not using a hardcoded mbuf size, add
  accessors for mbuf pool private area, add a helper to create a
  mbuf pool)

Changes in v3:
- a mbuf can now attach to another one that have a different private
  size. In this case, the m->priv_size corresponds to the size of the
  private area of the direct mbuf.
- add comments to reflect these changes
- minor style modifications

Changes in v2:
- do not change the use of MBUF_EXT_MEM() in vhost
- change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
  one parameter
- fix and rework rte_pktmbuf_detach()
- move m->priv_size in second mbuf cache line
- fix mbuf free in test error case

Olivier Matz (12):
  mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
  examples: always initialize mbuf_pool private area
  mbuf: add accessors to get data room size and private size
  mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
  testpmd: use standard functions to initialize mbufs and mbuf pool
  mbuf: introduce a new helper to create a mbuf pool
  apps: use rte_pktmbuf_pool_create to create mbuf pools
  mbuf: fix clone support when application uses private mbuf data
  mbuf: allow to clone an indirect mbuf
  test/mbuf: rename mc variable in m
  test/mbuf: enhance mbuf refcnt test
  test/mbuf: verify that cloning a clone works properly

 app/test-pipeline/init.c                           |  15 +-
 app/test-pmd/testpmd.c                             |  78 +--------
 app/test/test_distributor.c                        |  10 +-
 app/test/test_distributor_perf.c                   |  10 +-
 app/test/test_kni.c                                |  16 +-
 app/test/test_link_bonding.c                       |  10 +-
 app/test/test_link_bonding_mode4.c                 |  12 +-
 app/test/test_mbuf.c                               | 110 +++++++++---
 app/test/test_pmd_perf.c                           |  11 +-
 app/test/test_pmd_ring.c                           |  10 +-
 app/test/test_reorder.c                            |  10 +-
 app/test/test_sched.c                              |  16 +-
 app/test/test_table.c                              |   9 +-
 app/test/test_table.h                              |   3 +-
 doc/guides/rel_notes/updating_apps.rst             |  16 ++
 examples/bond/main.c                               |  10 +-
 examples/distributor/main.c                        |  11 +-
 examples/dpdk_qat/main.c                           |  10 +-
 examples/exception_path/main.c                     |  14 +-
 examples/ip_fragmentation/main.c                   |  18 +-
 examples/ip_pipeline/init.c                        |  28 +--
 examples/ipv4_multicast/main.c                     |  21 +--
 examples/kni/main.c                                |  12 +-
 examples/l2fwd-ivshmem/host/host.c                 |  10 +-
 examples/l2fwd-jobstats/main.c                     |  10 +-
 examples/l2fwd/main.c                              |  11 +-
 examples/l3fwd-acl/main.c                          |  11 +-
 examples/l3fwd-power/main.c                        |  11 +-
 examples/l3fwd-vf/main.c                           |  12 +-
 examples/l3fwd/main.c                              |  10 +-
 examples/link_status_interrupt/main.c              |  10 +-
 examples/load_balancer/init.c                      |  12 +-
 examples/load_balancer/main.h                      |   4 +-
 .../client_server_mp/mp_server/init.c              |  10 +-
 examples/multi_process/symmetric_mp/main.c         |  10 +-
 examples/netmap_compat/bridge/bridge.c             |  12 +-
 examples/packet_ordering/main.c                    |  11 +-
 examples/qos_meter/main.c                          |   7 +-
 examples/qos_sched/init.c                          |  10 +-
 examples/qos_sched/main.h                          |   2 +-
 examples/quota_watermark/include/conf.h            |   2 +-
 examples/quota_watermark/qw/main.c                 |   7 +-
 examples/rxtx_callbacks/main.c                     |  11 +-
 examples/skeleton/basicfwd.c                       |  13 +-
 examples/vhost/main.c                              |  31 ++--
 examples/vhost_xen/main.c                          |  11 +-
 examples/vmdq/main.c                               |  11 +-
 examples/vmdq_dcb/main.c                           |  10 +-
 lib/librte_ether/rte_ethdev.c                      |   4 +-
 lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
 lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
 lib/librte_mbuf/rte_mbuf_version.map               |   8 +
 lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
 lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
 lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
 lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
 lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
 lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
 lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
 lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
 lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
 62 files changed, 507 insertions(+), 566 deletions(-)
Neil Horman April 21, 2015, 11:50 a.m. UTC | #16 
On Tue, Apr 21, 2015 at 11:55:10AM +0200, Olivier Matz wrote:
> The first objective of this series is to fix the support of indirect
> mbufs when the application reserves a private area in mbufs. It also
> removes the limitation that rte_pktmbuf_clone() is only allowed on
> direct (non-cloned) mbufs. The series also contains some enhancements
> and fixes in the mbuf area that makes the implementation of the
> last patches easier.
> 
> Changes in v5:
> - update rte_mbuf_version.map to fix compilation with shared libraries
> 
> Changes in v4:
> - do not add a priv_size in mbuf structure, having a proper accessor
>   to read it from the pool private area is clearer
> - prepend some reworks in the mbuf area to simplify the implementation
>   (fix mbuf initialization by not using a hardcoded mbuf size, add
>   accessors for mbuf pool private area, add a helper to create a
>   mbuf pool)
> 
> Changes in v3:
> - a mbuf can now attach to another one that have a different private
>   size. In this case, the m->priv_size corresponds to the size of the
>   private area of the direct mbuf.
> - add comments to reflect these changes
> - minor style modifications
> 
> Changes in v2:
> - do not change the use of MBUF_EXT_MEM() in vhost
> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
>   one parameter
> - fix and rework rte_pktmbuf_detach()
> - move m->priv_size in second mbuf cache line
> - fix mbuf free in test error case
> 
> Olivier Matz (12):
>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>   examples: always initialize mbuf_pool private area
>   mbuf: add accessors to get data room size and private size
>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>   testpmd: use standard functions to initialize mbufs and mbuf pool
>   mbuf: introduce a new helper to create a mbuf pool
>   apps: use rte_pktmbuf_pool_create to create mbuf pools
>   mbuf: fix clone support when application uses private mbuf data
>   mbuf: allow to clone an indirect mbuf
>   test/mbuf: rename mc variable in m
>   test/mbuf: enhance mbuf refcnt test
>   test/mbuf: verify that cloning a clone works properly
> 
>  app/test-pipeline/init.c                           |  15 +-
>  app/test-pmd/testpmd.c                             |  78 +--------
>  app/test/test_distributor.c                        |  10 +-
>  app/test/test_distributor_perf.c                   |  10 +-
>  app/test/test_kni.c                                |  16 +-
>  app/test/test_link_bonding.c                       |  10 +-
>  app/test/test_link_bonding_mode4.c                 |  12 +-
>  app/test/test_mbuf.c                               | 110 +++++++++---
>  app/test/test_pmd_perf.c                           |  11 +-
>  app/test/test_pmd_ring.c                           |  10 +-
>  app/test/test_reorder.c                            |  10 +-
>  app/test/test_sched.c                              |  16 +-
>  app/test/test_table.c                              |   9 +-
>  app/test/test_table.h                              |   3 +-
>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
>  examples/bond/main.c                               |  10 +-
>  examples/distributor/main.c                        |  11 +-
>  examples/dpdk_qat/main.c                           |  10 +-
>  examples/exception_path/main.c                     |  14 +-
>  examples/ip_fragmentation/main.c                   |  18 +-
>  examples/ip_pipeline/init.c                        |  28 +--
>  examples/ipv4_multicast/main.c                     |  21 +--
>  examples/kni/main.c                                |  12 +-
>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
>  examples/l2fwd-jobstats/main.c                     |  10 +-
>  examples/l2fwd/main.c                              |  11 +-
>  examples/l3fwd-acl/main.c                          |  11 +-
>  examples/l3fwd-power/main.c                        |  11 +-
>  examples/l3fwd-vf/main.c                           |  12 +-
>  examples/l3fwd/main.c                              |  10 +-
>  examples/link_status_interrupt/main.c              |  10 +-
>  examples/load_balancer/init.c                      |  12 +-
>  examples/load_balancer/main.h                      |   4 +-
>  .../client_server_mp/mp_server/init.c              |  10 +-
>  examples/multi_process/symmetric_mp/main.c         |  10 +-
>  examples/netmap_compat/bridge/bridge.c             |  12 +-
>  examples/packet_ordering/main.c                    |  11 +-
>  examples/qos_meter/main.c                          |   7 +-
>  examples/qos_sched/init.c                          |  10 +-
>  examples/qos_sched/main.h                          |   2 +-
>  examples/quota_watermark/include/conf.h            |   2 +-
>  examples/quota_watermark/qw/main.c                 |   7 +-
>  examples/rxtx_callbacks/main.c                     |  11 +-
>  examples/skeleton/basicfwd.c                       |  13 +-
>  examples/vhost/main.c                              |  31 ++--
>  examples/vhost_xen/main.c                          |  11 +-
>  examples/vmdq/main.c                               |  11 +-
>  examples/vmdq_dcb/main.c                           |  10 +-
>  lib/librte_ether/rte_ethdev.c                      |   4 +-
>  lib/librte_mbuf/rte_mbuf.c                         |  63 +++++--
>  lib/librte_mbuf/rte_mbuf.h                         | 189 ++++++++++++++++-----
>  lib/librte_mbuf/rte_mbuf_version.map               |   8 +
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
>  62 files changed, 507 insertions(+), 566 deletions(-)
> 
> -- 
> 2.1.4
> 
> 


applies,builds, maintains ABI.  Seems to work in test app cases.
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Olivier Matz April 22, 2015, 9:57 a.m. UTC | #17 
The first objective of this series is to fix the support of indirect
mbufs when the application reserves a private area in mbufs. It also
removes the limitation that rte_pktmbuf_clone() is only allowed on
direct (non-cloned) mbufs. The series also contains some enhancements
and fixes in the mbuf area that makes the implementation of the
last patches easier.

Changes in v6:
- restore the priv_size in mbuf structure, version 4 broke the
  attachment between mbufs having different private size
- add a test case to ensure it won't be broken again
- replace 0xffff by UINT16_MAX
- fix some minor checkpatch issues

Changes in v5:
- update rte_mbuf_version.map to fix compilation with shared libraries

Changes in v4:
- do not add a priv_size in mbuf structure, having a proper accessor
  to read it from the pool private area is clearer
- prepend some reworks in the mbuf area to simplify the implementation
  (fix mbuf initialization by not using a hardcoded mbuf size, add
  accessors for mbuf pool private area, add a helper to create a
  mbuf pool)

Changes in v3:
- a mbuf can now attach to another one that have a different private
  size. In this case, the m->priv_size corresponds to the size of the
  private area of the direct mbuf.
- add comments to reflect these changes
- minor style modifications

Changes in v2:
- do not change the use of MBUF_EXT_MEM() in vhost
- change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
  one parameter
- fix and rework rte_pktmbuf_detach()
- move m->priv_size in second mbuf cache line
- fix mbuf free in test error case

Olivier Matz (13):
  mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
  examples: always initialize mbuf_pool private area
  mbuf: add accessors to get data room size and private size
  mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
  testpmd: use standard functions to initialize mbufs and mbuf pool
  mbuf: introduce a new helper to create a mbuf pool
  apps: use rte_pktmbuf_pool_create to create mbuf pools
  mbuf: fix clone support when application uses private mbuf data
  mbuf: allow to clone an indirect mbuf
  test/mbuf: rename mc variable in m
  test/mbuf: enhance mbuf refcnt test
  test/mbuf: verify that cloning a clone works properly
  test/mbuf: add a test case for clone with different priv size

 app/test-pipeline/init.c                           |  15 +-
 app/test-pmd/testpmd.c                             |  84 +-------
 app/test/test_distributor.c                        |  10 +-
 app/test/test_distributor_perf.c                   |  10 +-
 app/test/test_kni.c                                |  16 +-
 app/test/test_link_bonding.c                       |  10 +-
 app/test/test_link_bonding_mode4.c                 |  12 +-
 app/test/test_mbuf.c                               | 238 ++++++++++++++++++---
 app/test/test_pmd_perf.c                           |  11 +-
 app/test/test_pmd_ring.c                           |  10 +-
 app/test/test_reorder.c                            |  10 +-
 app/test/test_sched.c                              |  16 +-
 app/test/test_table.c                              |   9 +-
 app/test/test_table.h                              |   3 +-
 doc/guides/rel_notes/updating_apps.rst             |  16 ++
 examples/bond/main.c                               |  10 +-
 examples/distributor/main.c                        |  11 +-
 examples/dpdk_qat/main.c                           |  10 +-
 examples/exception_path/main.c                     |  14 +-
 examples/ip_fragmentation/main.c                   |  18 +-
 examples/ip_pipeline/init.c                        |  28 +--
 examples/ipv4_multicast/main.c                     |  21 +-
 examples/kni/main.c                                |  12 +-
 examples/l2fwd-ivshmem/host/host.c                 |  10 +-
 examples/l2fwd-jobstats/main.c                     |  10 +-
 examples/l2fwd/main.c                              |  11 +-
 examples/l3fwd-acl/main.c                          |  11 +-
 examples/l3fwd-power/main.c                        |  11 +-
 examples/l3fwd-vf/main.c                           |  12 +-
 examples/l3fwd/main.c                              |  10 +-
 examples/link_status_interrupt/main.c              |  10 +-
 examples/load_balancer/init.c                      |  12 +-
 examples/load_balancer/main.h                      |   4 +-
 .../client_server_mp/mp_server/init.c              |  10 +-
 examples/multi_process/symmetric_mp/main.c         |  10 +-
 examples/netmap_compat/bridge/bridge.c             |  12 +-
 examples/packet_ordering/main.c                    |  11 +-
 examples/qos_meter/main.c                          |   7 +-
 examples/qos_sched/init.c                          |  10 +-
 examples/qos_sched/main.h                          |   2 +-
 examples/quota_watermark/include/conf.h            |   2 +-
 examples/quota_watermark/qw/main.c                 |   7 +-
 examples/rxtx_callbacks/main.c                     |  11 +-
 examples/skeleton/basicfwd.c                       |  13 +-
 examples/vhost/main.c                              |  31 +--
 examples/vhost_xen/main.c                          |  11 +-
 examples/vmdq/main.c                               |  11 +-
 examples/vmdq_dcb/main.c                           |  10 +-
 lib/librte_ether/rte_ethdev.c                      |   4 +-
 lib/librte_mbuf/rte_mbuf.c                         |  65 ++++--
 lib/librte_mbuf/rte_mbuf.h                         | 200 +++++++++++++----
 lib/librte_mbuf/rte_mbuf_version.map               |   8 +
 lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
 lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
 lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
 lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
 lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
 lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
 lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
 lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
 lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
 62 files changed, 650 insertions(+), 570 deletions(-)
Ananyev, Konstantin April 22, 2015, 11:59 a.m. UTC | #18 
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, April 22, 2015 10:57 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
> Subject: [PATCH v6 00/13] mbuf: enhancements of mbuf clones
> 
> The first objective of this series is to fix the support of indirect
> mbufs when the application reserves a private area in mbufs. It also
> removes the limitation that rte_pktmbuf_clone() is only allowed on
> direct (non-cloned) mbufs. The series also contains some enhancements
> and fixes in the mbuf area that makes the implementation of the
> last patches easier.
> 
> Changes in v6:
> - restore the priv_size in mbuf structure, version 4 broke the
>   attachment between mbufs having different private size
> - add a test case to ensure it won't be broken again
> - replace 0xffff by UINT16_MAX
> - fix some minor checkpatch issues
> 
> Changes in v5:
> - update rte_mbuf_version.map to fix compilation with shared libraries
> 
> Changes in v4:
> - do not add a priv_size in mbuf structure, having a proper accessor
>   to read it from the pool private area is clearer
> - prepend some reworks in the mbuf area to simplify the implementation
>   (fix mbuf initialization by not using a hardcoded mbuf size, add
>   accessors for mbuf pool private area, add a helper to create a
>   mbuf pool)
> 
> Changes in v3:
> - a mbuf can now attach to another one that have a different private
>   size. In this case, the m->priv_size corresponds to the size of the
>   private area of the direct mbuf.
> - add comments to reflect these changes
> - minor style modifications
> 
> Changes in v2:
> - do not change the use of MBUF_EXT_MEM() in vhost
> - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
>   one parameter
> - fix and rework rte_pktmbuf_detach()
> - move m->priv_size in second mbuf cache line
> - fix mbuf free in test error case
> 
> Olivier Matz (13):
>   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
>   examples: always initialize mbuf_pool private area
>   mbuf: add accessors to get data room size and private size
>   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
>   testpmd: use standard functions to initialize mbufs and mbuf pool
>   mbuf: introduce a new helper to create a mbuf pool
>   apps: use rte_pktmbuf_pool_create to create mbuf pools
>   mbuf: fix clone support when application uses private mbuf data
>   mbuf: allow to clone an indirect mbuf
>   test/mbuf: rename mc variable in m
>   test/mbuf: enhance mbuf refcnt test
>   test/mbuf: verify that cloning a clone works properly
>   test/mbuf: add a test case for clone with different priv size
> 
>  app/test-pipeline/init.c                           |  15 +-
>  app/test-pmd/testpmd.c                             |  84 +-------
>  app/test/test_distributor.c                        |  10 +-
>  app/test/test_distributor_perf.c                   |  10 +-
>  app/test/test_kni.c                                |  16 +-
>  app/test/test_link_bonding.c                       |  10 +-
>  app/test/test_link_bonding_mode4.c                 |  12 +-
>  app/test/test_mbuf.c                               | 238 ++++++++++++++++++---
>  app/test/test_pmd_perf.c                           |  11 +-
>  app/test/test_pmd_ring.c                           |  10 +-
>  app/test/test_reorder.c                            |  10 +-
>  app/test/test_sched.c                              |  16 +-
>  app/test/test_table.c                              |   9 +-
>  app/test/test_table.h                              |   3 +-
>  doc/guides/rel_notes/updating_apps.rst             |  16 ++
>  examples/bond/main.c                               |  10 +-
>  examples/distributor/main.c                        |  11 +-
>  examples/dpdk_qat/main.c                           |  10 +-
>  examples/exception_path/main.c                     |  14 +-
>  examples/ip_fragmentation/main.c                   |  18 +-
>  examples/ip_pipeline/init.c                        |  28 +--
>  examples/ipv4_multicast/main.c                     |  21 +-
>  examples/kni/main.c                                |  12 +-
>  examples/l2fwd-ivshmem/host/host.c                 |  10 +-
>  examples/l2fwd-jobstats/main.c                     |  10 +-
>  examples/l2fwd/main.c                              |  11 +-
>  examples/l3fwd-acl/main.c                          |  11 +-
>  examples/l3fwd-power/main.c                        |  11 +-
>  examples/l3fwd-vf/main.c                           |  12 +-
>  examples/l3fwd/main.c                              |  10 +-
>  examples/link_status_interrupt/main.c              |  10 +-
>  examples/load_balancer/init.c                      |  12 +-
>  examples/load_balancer/main.h                      |   4 +-
>  .../client_server_mp/mp_server/init.c              |  10 +-
>  examples/multi_process/symmetric_mp/main.c         |  10 +-
>  examples/netmap_compat/bridge/bridge.c             |  12 +-
>  examples/packet_ordering/main.c                    |  11 +-
>  examples/qos_meter/main.c                          |   7 +-
>  examples/qos_sched/init.c                          |  10 +-
>  examples/qos_sched/main.h                          |   2 +-
>  examples/quota_watermark/include/conf.h            |   2 +-
>  examples/quota_watermark/qw/main.c                 |   7 +-
>  examples/rxtx_callbacks/main.c                     |  11 +-
>  examples/skeleton/basicfwd.c                       |  13 +-
>  examples/vhost/main.c                              |  31 +--
>  examples/vhost_xen/main.c                          |  11 +-
>  examples/vmdq/main.c                               |  11 +-
>  examples/vmdq_dcb/main.c                           |  10 +-
>  lib/librte_ether/rte_ethdev.c                      |   4 +-
>  lib/librte_mbuf/rte_mbuf.c                         |  65 ++++--
>  lib/librte_mbuf/rte_mbuf.h                         | 200 +++++++++++++----
>  lib/librte_mbuf/rte_mbuf_version.map               |   8 +
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c       |   6 +-
>  lib/librte_pmd_bond/rte_eth_bond_alb.c             |  16 +-
>  lib/librte_pmd_e1000/em_rxtx.c                     |   5 +-
>  lib/librte_pmd_e1000/igb_rxtx.c                    |  12 +-
>  lib/librte_pmd_fm10k/fm10k_ethdev.c                |   6 +-
>  lib/librte_pmd_i40e/i40e_ethdev_vf.c               |   6 +-
>  lib/librte_pmd_i40e/i40e_rxtx.c                    |  15 +-
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c                  |  12 +-
>  lib/librte_pmd_pcap/rte_eth_pcap.c                 |   5 +-
>  lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c              |   7 +-
>  62 files changed, 650 insertions(+), 570 deletions(-)
> 

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 2.1.4
Zoltan Kiss April 24, 2015, 10:38 a.m. UTC | #19 
Hi,

On 22/04/15 12:59, Ananyev, Konstantin wrote:
>
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Wednesday, April 22, 2015 10:57 AM
>> To: dev@dpdk.org
>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
>> Subject: [PATCH v6 00/13] mbuf: enhancements of mbuf clones
>>
>> The first objective of this series is to fix the support of indirect
>> mbufs when the application reserves a private area in mbufs. It also
>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>> direct (non-cloned) mbufs. The series also contains some enhancements
>> and fixes in the mbuf area that makes the implementation of the
>> last patches easier.
>>
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

When does this series get merged?

Regards,

Zoltan
Thomas Monjalon April 27, 2015, 5:38 p.m. UTC | #20 
2015-04-24 11:38, Zoltan Kiss:
> On 22/04/15 12:59, Ananyev, Konstantin wrote:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Wednesday, April 22, 2015 10:57 AM
> >> The first objective of this series is to fix the support of indirect
> >> mbufs when the application reserves a private area in mbufs. It also
> >> removes the limitation that rte_pktmbuf_clone() is only allowed on
> >> direct (non-cloned) mbufs. The series also contains some enhancements
> >> and fixes in the mbuf area that makes the implementation of the
> >> last patches easier.
> >
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> When does this series get merged?

It was acked on April 22  and your question was 2 days later on April 24.
Does it mean you are expecting it to be merged the day it is acked?
Or do you fear the merging because of a local dev you are working on?
Anyway, everybody seems happy with this version so it's going to be merged.
Thomas Monjalon April 28, 2015, 9:50 a.m. UTC | #21 
> > The first objective of this series is to fix the support of indirect
> > mbufs when the application reserves a private area in mbufs. It also
> > removes the limitation that rte_pktmbuf_clone() is only allowed on
> > direct (non-cloned) mbufs. The series also contains some enhancements
> > and fixes in the mbuf area that makes the implementation of the
> > last patches easier.
> > 
> > Changes in v6:
> > - restore the priv_size in mbuf structure, version 4 broke the
> >   attachment between mbufs having different private size
> > - add a test case to ensure it won't be broken again
> > - replace 0xffff by UINT16_MAX
> > - fix some minor checkpatch issues
> > 
> > Changes in v5:
> > - update rte_mbuf_version.map to fix compilation with shared libraries
> > 
> > Changes in v4:
> > - do not add a priv_size in mbuf structure, having a proper accessor
> >   to read it from the pool private area is clearer
> > - prepend some reworks in the mbuf area to simplify the implementation
> >   (fix mbuf initialization by not using a hardcoded mbuf size, add
> >   accessors for mbuf pool private area, add a helper to create a
> >   mbuf pool)
> > 
> > Changes in v3:
> > - a mbuf can now attach to another one that have a different private
> >   size. In this case, the m->priv_size corresponds to the size of the
> >   private area of the direct mbuf.
> > - add comments to reflect these changes
> > - minor style modifications
> > 
> > Changes in v2:
> > - do not change the use of MBUF_EXT_MEM() in vhost
> > - change rte_mbuf_from_baddr() to rte_mbuf_from_indirect(), removing
> >   one parameter
> > - fix and rework rte_pktmbuf_detach()
> > - move m->priv_size in second mbuf cache line
> > - fix mbuf free in test error case
> > 
> > Olivier Matz (13):
> >   mbuf: fix mbuf data room size calculation rte_pktmbuf_pool_init
> >   examples: always initialize mbuf_pool private area
> >   mbuf: add accessors to get data room size and private size
> >   mbuf: fix rte_pktmbuf_init when mbuf private size is not zero
> >   testpmd: use standard functions to initialize mbufs and mbuf pool
> >   mbuf: introduce a new helper to create a mbuf pool
> >   apps: use rte_pktmbuf_pool_create to create mbuf pools
> >   mbuf: fix clone support when application uses private mbuf data
> >   mbuf: allow to clone an indirect mbuf
> >   test/mbuf: rename mc variable in m
> >   test/mbuf: enhance mbuf refcnt test
> >   test/mbuf: verify that cloning a clone works properly
> >   test/mbuf: add a test case for clone with different priv size
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks
Zoltan Kiss April 28, 2015, 11:15 a.m. UTC | #22 
On 27/04/15 18:38, Thomas Monjalon wrote:
> 2015-04-24 11:38, Zoltan Kiss:
>> On 22/04/15 12:59, Ananyev, Konstantin wrote:
>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>> Sent: Wednesday, April 22, 2015 10:57 AM
>>>> The first objective of this series is to fix the support of indirect
>>>> mbufs when the application reserves a private area in mbufs. It also
>>>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>>>> direct (non-cloned) mbufs. The series also contains some enhancements
>>>> and fixes in the mbuf area that makes the implementation of the
>>>> last patches easier.
>>>
>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>
>> When does this series get merged?
>
> It was acked on April 22  and your question was 2 days later on April 24.
> Does it mean you are expecting it to be merged the day it is acked?

I was just curious about when to expect it, so I can plan to do some 
further work based on it, but nothing pressing.

Regards,

Zoltan

> Or do you fear the merging because of a local dev you are working on?
> Anyway, everybody seems happy with this version so it's going to be merged.
>
Xu, HuilongX May 7, 2015, 1:57 a.m. UTC | #23 
Hi Olivier,
Today I find a compile error, when I test ip fragment on dpdk.org. would you check this?  thanks a lot.
My dpdk.org commit: a6d71fa7146cc04320c2485d6dde44c1d888d652
The compile error as below:
CC main.o
/root/dpdk/examples/ip_fragmentation/main.c: In function 'init_mem':
/root/dpdk/examples/ip_fragmentation/main.c:748:8: error: 'MBUF_DATA_SIZE' undeclared (first use in this function)
     0, MBUF_DATA_SIZE, socket);
        ^
/root/dpdk/examples/ip_fragmentation/main.c:748:8: note: each undeclared identifier is reported only once for each function it appears in


Best regards

huilong

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
Sent: Friday, April 24, 2015 6:39 PM
To: Ananyev, Konstantin; Olivier Matz; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v6 00/13] mbuf: enhancements of mbuf clones

Hi,

On 22/04/15 12:59, Ananyev, Konstantin wrote:
>
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Wednesday, April 22, 2015 10:57 AM
>> To: dev@dpdk.org
>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
>> Subject: [PATCH v6 00/13] mbuf: enhancements of mbuf clones
>>
>> The first objective of this series is to fix the support of indirect
>> mbufs when the application reserves a private area in mbufs. It also
>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>> direct (non-cloned) mbufs. The series also contains some enhancements
>> and fixes in the mbuf area that makes the implementation of the
>> last patches easier.
>>
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

When does this series get merged?

Regards,

Zoltan
Olivier Matz May 7, 2015, 7:32 a.m. UTC | #24 
Hi Huilong,

On 05/07/2015 03:57 AM, Xu, HuilongX wrote:
> Hi Olivier,
> Today I find a compile error, when I test ip fragment on dpdk.org. would you check this?  thanks a lot.
> My dpdk.org commit: a6d71fa7146cc04320c2485d6dde44c1d888d652
> The compile error as below:
> CC main.o
> /root/dpdk/examples/ip_fragmentation/main.c: In function 'init_mem':
> /root/dpdk/examples/ip_fragmentation/main.c:748:8: error: 'MBUF_DATA_SIZE' undeclared (first use in this function)
>       0, MBUF_DATA_SIZE, socket);
>          ^
> /root/dpdk/examples/ip_fragmentation/main.c:748:8: note: each undeclared identifier is reported only once for each function it appears in

Sure, I'll have a look. Thanks for reporting.

Regards,
Olivier


>
>
> Best regards
>
> huilong
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
> Sent: Friday, April 24, 2015 6:39 PM
> To: Ananyev, Konstantin; Olivier Matz; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 00/13] mbuf: enhancements of mbuf clones
>
> Hi,
>
> On 22/04/15 12:59, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>> Sent: Wednesday, April 22, 2015 10:57 AM
>>> To: dev@dpdk.org
>>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
>>> Subject: [PATCH v6 00/13] mbuf: enhancements of mbuf clones
>>>
>>> The first objective of this series is to fix the support of indirect
>>> mbufs when the application reserves a private area in mbufs. It also
>>> removes the limitation that rte_pktmbuf_clone() is only allowed on
>>> direct (non-cloned) mbufs. The series also contains some enhancements
>>> and fixes in the mbuf area that makes the implementation of the
>>> last patches easier.
>>>
>>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> When does this series get merged?
>
> Regards,
>
> Zoltan
>
Ananyev, Konstantin May 7, 2015, 9:39 a.m. UTC | #25 
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, May 07, 2015 8:32 AM
> To: Xu, HuilongX; Zoltan Kiss; Ananyev, Konstantin; dev@dpdk.org
> Cc: Cao, Waterman; Cao, Min; Zhang, Helin
> Subject: Re: [dpdk-dev] [PATCH v6 00/13] mbuf: enhancements of mbuf clones
> 
> Hi Huilong,
> 
> On 05/07/2015 03:57 AM, Xu, HuilongX wrote:
> > Hi Olivier,
> > Today I find a compile error, when I test ip fragment on dpdk.org. would you check this?  thanks a lot.
> > My dpdk.org commit: a6d71fa7146cc04320c2485d6dde44c1d888d652
> > The compile error as below:
> > CC main.o
> > /root/dpdk/examples/ip_fragmentation/main.c: In function 'init_mem':
> > /root/dpdk/examples/ip_fragmentation/main.c:748:8: error: 'MBUF_DATA_SIZE' undeclared (first use in this function)
> >       0, MBUF_DATA_SIZE, socket);
> >          ^
> > /root/dpdk/examples/ip_fragmentation/main.c:748:8: note: each undeclared identifier is reported only once for each function it
> appears in
> 
> Sure, I'll have a look. Thanks for reporting.

Looks like a typo here.
Should be fixed by http://dpdk.org/ml/archives/dev/2015-April/017119.html.
Konstantin

> 
> Regards,
> Olivier
> 
> 
> >
> >
> > Best regards
> >
> > huilong
> >
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
> > Sent: Friday, April 24, 2015 6:39 PM
> > To: Ananyev, Konstantin; Olivier Matz; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 00/13] mbuf: enhancements of mbuf clones
> >
> > Hi,
> >
> > On 22/04/15 12:59, Ananyev, Konstantin wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >>> Sent: Wednesday, April 22, 2015 10:57 AM
> >>> To: dev@dpdk.org
> >>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
> >>> Subject: [PATCH v6 00/13] mbuf: enhancements of mbuf clones
> >>>
> >>> The first objective of this series is to fix the support of indirect
> >>> mbufs when the application reserves a private area in mbufs. It also
> >>> removes the limitation that rte_pktmbuf_clone() is only allowed on
> >>> direct (non-cloned) mbufs. The series also contains some enhancements
> >>> and fixes in the mbuf area that makes the implementation of the
> >>> last patches easier.
> >>>
> >>
> >> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >
> > When does this series get merged?
> >
> > Regards,
> >
> > Zoltan
> >
diff mbox

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 3057791..c5a195a 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -425,6 +425,7 @@  testpmd_mbuf_ctor(struct rte_mempool *mp,
 	mb->tx_offload   = 0;
 	mb->vlan_tci     = 0;
 	mb->hash.rss     = 0;
+	mb->priv_size    = 0;
 }
 
 static void
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index c3fcb80..e44e82f 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -139,7 +139,7 @@ 
 /* Number of descriptors per cacheline. */
 #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
 
-#define MBUF_EXT_MEM(mb)   (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb))
+#define MBUF_EXT_MEM(mb)   (rte_mbuf_from_indirect(mb) != (mb))
 
 /* mask of enabled ports */
 static uint32_t enabled_port_mask = 0;
@@ -1550,7 +1550,7 @@  attach_rxmbuf_zcp(struct virtio_net *dev)
 static inline void pktmbuf_detach_zcp(struct rte_mbuf *m)
 {
 	const struct rte_mempool *mp = m->pool;
-	void *buf = RTE_MBUF_TO_BADDR(m);
+	void *buf = rte_mbuf_to_baddr(m);
 	uint32_t buf_ofs;
 	uint32_t buf_len = mp->elt_size - sizeof(*m);
 	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m);
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 526b18d..e095999 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -125,6 +125,7 @@  rte_pktmbuf_init(struct rte_mempool *mp,
 	m->pool = mp;
 	m->nb_segs = 1;
 	m->port = 0xff;
+	m->priv_size = 0;
 }
 
 /* do some sanity checks on a mbuf: panic if it fails */
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 17ba791..932fe58 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -317,18 +317,51 @@  struct rte_mbuf {
 			/* uint64_t unused:8; */
 		};
 	};
+
+	/** Size of the application private data. In case of an indirect
+	 * mbuf, it stores the direct mbuf private data size. */
+	uint16_t priv_size;
 } __rte_cache_aligned;
 
 /**
- * Given the buf_addr returns the pointer to corresponding mbuf.
+ * Return the mbuf owning the data buffer address of an indirect mbuf.
+ *
+ * @param mi
+ *   The pointer to the indirect mbuf.
+ * @return
+ *   The address of the direct mbuf corresponding to buffer_addr.
  */
-#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
+static inline struct rte_mbuf *
+rte_mbuf_from_indirect(struct rte_mbuf *mi)
+{
+       struct rte_mbuf *md;
+
+       /* mi->buf_addr and mi->priv_size correspond to buffer and
+	* private size of the direct mbuf */
+       md = (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) -
+	       mi->priv_size);
+       return md;
+}
 
 /**
- * Given the pointer to mbuf returns an address where it's  buf_addr
- * should point to.
+ * Return the buffer address embedded in the given mbuf.
+ *
+ * The user must ensure that m->priv_size corresponds to the
+ * private size of this mbuf, which is not the case for indirect
+ * mbufs.
+ *
+ * @param md
+ *   The pointer to the mbuf.
+ * @return
+ *   The address of the data buffer owned by the mbuf.
  */
-#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
+static inline char *
+rte_mbuf_to_baddr(struct rte_mbuf *md)
+{
+       char *buffer_addr;
+       buffer_addr = (char *)md + sizeof(*md) + md->priv_size;
+       return buffer_addr;
+}
 
 /**
  * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
@@ -688,6 +721,7 @@  static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
 
 /**
  * Attach packet mbuf to another packet mbuf.
+ *
  * After attachment we refer the mbuf we attached as 'indirect',
  * while mbuf we attached to as 'direct'.
  * Right now, not supported:
@@ -701,7 +735,6 @@  static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
  * @param md
  *   The direct packet mbuf.
  */
-
 static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
 {
 	RTE_MBUF_ASSERT(RTE_MBUF_DIRECT(md) &&
@@ -712,6 +745,7 @@  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
 	mi->buf_physaddr = md->buf_physaddr;
 	mi->buf_addr = md->buf_addr;
 	mi->buf_len = md->buf_len;
+	mi->priv_size = md->priv_size;
 
 	mi->next = md->next;
 	mi->data_off = md->data_off;
@@ -732,7 +766,8 @@  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
 }
 
 /**
- * Detach an indirect packet mbuf -
+ * Detach an indirect packet mbuf.
+ *
  *  - restore original mbuf address and length values.
  *  - reset pktmbuf data and data_len to their default values.
  *  All other fields of the given packet mbuf will be left intact.
@@ -740,22 +775,28 @@  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
  * @param m
  *   The indirect attached packet mbuf.
  */
-
 static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 {
-	const struct rte_mempool *mp = m->pool;
-	void *buf = RTE_MBUF_TO_BADDR(m);
-	uint32_t buf_len = mp->elt_size - sizeof(*m);
-	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
-
+	struct rte_pktmbuf_pool_private *mbp_priv;
+	struct rte_mempool *mp = m->pool;
+	void *buf;
+	unsigned mhdr_size;
+
+	/* first, restore the priv_size, this is needed before calling
+	 * rte_mbuf_to_baddr() */
+	mbp_priv = rte_mempool_get_priv(mp);
+	m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM -
+		mbp_priv->mbuf_data_room_size -
+		sizeof(struct rte_mbuf);
+
+	buf = rte_mbuf_to_baddr(m);
+	mhdr_size = (char *)buf - (char *)m;
+	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
 	m->buf_addr = buf;
-	m->buf_len = (uint16_t)buf_len;
-
+	m->buf_len = (uint16_t)(mp->elt_size - mhdr_size);
 	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
 			RTE_PKTMBUF_HEADROOM : m->buf_len;
-
 	m->data_len = 0;
-
 	m->ol_flags = 0;
 }
 
@@ -774,7 +815,7 @@  __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 		 *  - free attached mbuf segment
 		 */
 		if (RTE_MBUF_INDIRECT(m)) {
-			struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr);
+			struct rte_mbuf *md = rte_mbuf_from_indirect(m);
 			rte_pktmbuf_detach(m);
 			if (rte_mbuf_refcnt_update(md, -1) == 0)
 				__rte_mbuf_raw_free(md);