[dpdk-dev,v5,08/12] mbuf: fix clone support when application uses private mbuf data

Message ID 1429610122-30943-9-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Olivier Matz April 21, 2015, 9:55 a.m. UTC
  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>
---
 examples/vhost/main.c      |  4 ++--
 lib/librte_mbuf/rte_mbuf.c |  2 +-
 lib/librte_mbuf/rte_mbuf.h | 59 +++++++++++++++++++++++++++++++---------------
 3 files changed, 43 insertions(+), 22 deletions(-)
  

Comments

Ananyev, Konstantin April 21, 2015, 3:01 p.m. UTC | #1
Hi Olivier,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, April 21, 2015 10:55 AM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
> Subject: [PATCH v5 08/12] mbuf: fix clone support when application uses private mbuf data
> 
> 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>
> ---
>  examples/vhost/main.c      |  4 ++--
>  lib/librte_mbuf/rte_mbuf.c |  2 +-
>  lib/librte_mbuf/rte_mbuf.h | 59 +++++++++++++++++++++++++++++++---------------
>  3 files changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 22d6a4b..195d82f 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -138,7 +138,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;
> @@ -1549,7 +1549,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 b013607..784ae8b 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -129,7 +129,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
> 
>  	memset(m, 0, mp->elt_size);
> 
> -	/* start of buffer is just after mbuf structure */
> +	/* start of buffer is after mbuf structure and priv data */
>  	m->buf_addr = (char *)m + mbuf_size;
>  	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
>  	m->buf_len = (uint16_t)buf_len;
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 42db8e3..5c01c5b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -320,16 +320,40 @@ struct rte_mbuf {
>  	};
>  } __rte_cache_aligned;
> 
> +static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
> +
>  /**
> - * 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;
> +       md = (struct rte_mbuf *)((char *)mi->buf_addr -
> +	       sizeof(*mi) - rte_pktmbuf_priv_size(mi->pool));
> +       return md;
> +}


Really puzzled, why do you remove prv_size from mbuf?
Now if let say we attach mbuf (mi) with priv_size == X to mbuf (md) with priv_size == Y,
then rte_mbuf_from_indirect(mi) would return: mi->buf_addr - 128 - X, while it should be: mi->buf_addr - 128 - Y.

Another thing, rte_mbuf_from_indirect() and rte_mbuf_to_baddr() would probably become a bit slower -
as another level of indirection is added.

Konstantin

> 
>  /**
> - * 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.
> + *
> + * @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) + rte_pktmbuf_priv_size(md->pool);
> +       return buffer_addr;
> +}
> 
>  /**
>   * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> @@ -771,6 +795,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:
> @@ -784,7 +809,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) &&
> @@ -815,7 +839,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.
> @@ -823,22 +848,18 @@ 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_mempool *mp = m->pool;
> +	uint32_t mbuf_size, buf_len;
> 
> -	m->buf_addr = buf;
> +	mbuf_size = sizeof(struct rte_mbuf) + rte_pktmbuf_priv_size(mp);
> +	buf_len = rte_pktmbuf_data_room_size(mp);
> +	m->buf_addr = rte_mbuf_to_baddr(m);
> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
>  	m->buf_len = (uint16_t)buf_len;
> -
> -	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
> -			RTE_PKTMBUF_HEADROOM : m->buf_len;
> -
> +	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>  	m->data_len = 0;
> -
>  	m->ol_flags = 0;
>  }
> 
> @@ -867,7 +888,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 21, 2015, 3:26 p.m. UTC | #2
Hi Konstantin,

On 04/21/2015 05:01 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, April 21, 2015 10:55 AM
>> To: dev@dpdk.org
>> Cc: Ananyev, Konstantin; zoltan.kiss@linaro.org; Richardson, Bruce; nhorman@tuxdriver.com; olivier.matz@6wind.com
>> Subject: [PATCH v5 08/12] mbuf: fix clone support when application uses private mbuf data
>>
>> 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>
>> ---
>>   examples/vhost/main.c      |  4 ++--
>>   lib/librte_mbuf/rte_mbuf.c |  2 +-
>>   lib/librte_mbuf/rte_mbuf.h | 59 +++++++++++++++++++++++++++++++---------------
>>   3 files changed, 43 insertions(+), 22 deletions(-)
>>
>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
>> index 22d6a4b..195d82f 100644
>> --- a/examples/vhost/main.c
>> +++ b/examples/vhost/main.c
>> @@ -138,7 +138,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;
>> @@ -1549,7 +1549,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 b013607..784ae8b 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -129,7 +129,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>>
>>   	memset(m, 0, mp->elt_size);
>>
>> -	/* start of buffer is just after mbuf structure */
>> +	/* start of buffer is after mbuf structure and priv data */
>>   	m->buf_addr = (char *)m + mbuf_size;
>>   	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
>>   	m->buf_len = (uint16_t)buf_len;
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 42db8e3..5c01c5b 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -320,16 +320,40 @@ struct rte_mbuf {
>>   	};
>>   } __rte_cache_aligned;
>>
>> +static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
>> +
>>   /**
>> - * 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;
>> +       md = (struct rte_mbuf *)((char *)mi->buf_addr -
>> +	       sizeof(*mi) - rte_pktmbuf_priv_size(mi->pool));
>> +       return md;
>> +}
>
>
> Really puzzled, why do you remove prv_size from mbuf?
> Now if let say we attach mbuf (mi) with priv_size == X to mbuf (md) with priv_size == Y,
> then rte_mbuf_from_indirect(mi) would return: mi->buf_addr - 128 - X, while it should be: mi->buf_addr - 128 - Y.
>
> Another thing, rte_mbuf_from_indirect() and rte_mbuf_to_baddr() would probably become a bit slower -
> as another level of indirection is added.

You're right, I forgot this use case, it cannot work like this.
I think I will add a new test case, it will avoid me to break
it again in a version of the next patch series :)

Thanks,
Olivier



>
> Konstantin
>
>>
>>   /**
>> - * 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.
>> + *
>> + * @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) + rte_pktmbuf_priv_size(md->pool);
>> +       return buffer_addr;
>> +}
>>
>>   /**
>>    * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
>> @@ -771,6 +795,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:
>> @@ -784,7 +809,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) &&
>> @@ -815,7 +839,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.
>> @@ -823,22 +848,18 @@ 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_mempool *mp = m->pool;
>> +	uint32_t mbuf_size, buf_len;
>>
>> -	m->buf_addr = buf;
>> +	mbuf_size = sizeof(struct rte_mbuf) + rte_pktmbuf_priv_size(mp);
>> +	buf_len = rte_pktmbuf_data_room_size(mp);
>> +	m->buf_addr = rte_mbuf_to_baddr(m);
>> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
>>   	m->buf_len = (uint16_t)buf_len;
>> -
>> -	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
>> -			RTE_PKTMBUF_HEADROOM : m->buf_len;
>> -
>> +	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
>>   	m->data_len = 0;
>> -
>>   	m->ol_flags = 0;
>>   }
>>
>> @@ -867,7 +888,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
>
  

Patch

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 22d6a4b..195d82f 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -138,7 +138,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;
@@ -1549,7 +1549,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 b013607..784ae8b 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -129,7 +129,7 @@  rte_pktmbuf_init(struct rte_mempool *mp,
 
 	memset(m, 0, mp->elt_size);
 
-	/* start of buffer is just after mbuf structure */
+	/* start of buffer is after mbuf structure and priv data */
 	m->buf_addr = (char *)m + mbuf_size;
 	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
 	m->buf_len = (uint16_t)buf_len;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 42db8e3..5c01c5b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -320,16 +320,40 @@  struct rte_mbuf {
 	};
 } __rte_cache_aligned;
 
+static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
+
 /**
- * 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;
+       md = (struct rte_mbuf *)((char *)mi->buf_addr -
+	       sizeof(*mi) - rte_pktmbuf_priv_size(mi->pool));
+       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.
+ *
+ * @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) + rte_pktmbuf_priv_size(md->pool);
+       return buffer_addr;
+}
 
 /**
  * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
@@ -771,6 +795,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:
@@ -784,7 +809,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) &&
@@ -815,7 +839,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.
@@ -823,22 +848,18 @@  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_mempool *mp = m->pool;
+	uint32_t mbuf_size, buf_len;
 
-	m->buf_addr = buf;
+	mbuf_size = sizeof(struct rte_mbuf) + rte_pktmbuf_priv_size(mp);
+	buf_len = rte_pktmbuf_data_room_size(mp);
+	m->buf_addr = rte_mbuf_to_baddr(m);
+	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mbuf_size;
 	m->buf_len = (uint16_t)buf_len;
-
-	m->data_off = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ?
-			RTE_PKTMBUF_HEADROOM : m->buf_len;
-
+	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
 	m->data_len = 0;
-
 	m->ol_flags = 0;
 }
 
@@ -867,7 +888,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);