[dpdk-dev] ovs-dpdk: placing the metadata

[dpdk-dev] ovs-dpdk: placing the metadata

[Zoltan Kiss]

Hi,

I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the 
packet metadata right after "struct rte_mbuf", and before the buffer data:

     /* start of buffer is just after mbuf structure */
     m->buf_addr = (char *)m + sizeof(struct dp_packet);

(struct dp_packet has the rte_mbuf as first member if DPDK enabled) 	

However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer 
should start right after the rte_mbuf:

/**
  * Given the buf_addr returns the pointer to corresponding mbuf.
  */
#define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)

/**
  * Given the pointer to mbuf returns an address where it's  buf_addr
  * should point to.
  */
#define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)

These macros are used for attaching/detaching mbuf's to each other. This 
is the way the code retrieves the direct buffer from an indirect one, 
and vica versa. I think if we want to keep the metadata feature (which I 
guess is quite important), we need to add a pointer to rte_mbuf, which 
helps the direct and indirect structs to find each other. Something like:

	struct rte_mbuf *attach;    /**< Points to the other buffer if this one
					 is (in)direct. Otherwise NULL.  */

What do you think?

Regards,

Zoltan Kiss

Re: [dpdk-dev] ovs-dpdk: placing the metadata

[Olivier MATZ]

Hi Zoltan,

On 03/24/2015 06:42 PM, Zoltan Kiss wrote:
> Hi,
>
> I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the
> packet metadata right after "struct rte_mbuf", and before the buffer data:
>
>      /* start of buffer is just after mbuf structure */
>      m->buf_addr = (char *)m + sizeof(struct dp_packet);
>
> (struct dp_packet has the rte_mbuf as first member if DPDK enabled)
>
> However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer
> should start right after the rte_mbuf:
>
> /**
>   * Given the buf_addr returns the pointer to corresponding mbuf.
>   */
> #define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>
> /**
>   * Given the pointer to mbuf returns an address where it's  buf_addr
>   * should point to.
>   */
> #define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>
> These macros are used for attaching/detaching mbuf's to each other. This
> is the way the code retrieves the direct buffer from an indirect one,
> and vica versa. I think if we want to keep the metadata feature (which I
> guess is quite important), we need to add a pointer to rte_mbuf, which
> helps the direct and indirect structs to find each other. Something like:
>
>      struct rte_mbuf *attach;    /**< Points to the other buffer if this
> one
>                       is (in)direct. Otherwise NULL.  */
>
> What do you think?

I've just sent a patch that should fix this issue.
http://dpdk.org/ml/archives/dev/2015-March/015722.html

Let me know if you have any comment on it.

Regards,
Olivier

Re: [dpdk-dev] ovs-dpdk: placing the metadata

[Zoltan Kiss]

Hi Olivier,

On 25/03/15 17:04, Olivier MATZ wrote:
> Hi Zoltan,
>
> On 03/24/2015 06:42 PM, Zoltan Kiss wrote:
>> Hi,
>>
>> I've noticed in lib/netdev-dpdk.c that __rte_pktmbuf_init() stores the
>> packet metadata right after "struct rte_mbuf", and before the buffer
>> data:
>>
>>      /* start of buffer is just after mbuf structure */
>>      m->buf_addr = (char *)m + sizeof(struct dp_packet);
>>
>> (struct dp_packet has the rte_mbuf as first member if DPDK enabled)
>>
>> However, lib/librte_mbuf/rte_mbuf.h seems to codify that the buffer
>> should start right after the rte_mbuf:
>>
>> /**
>>   * Given the buf_addr returns the pointer to corresponding mbuf.
>>   */
>> #define RTE_MBUF_FROM_BADDR(ba)     (((struct rte_mbuf *)(ba)) - 1)
>>
>> /**
>>   * Given the pointer to mbuf returns an address where it's  buf_addr
>>   * should point to.
>>   */
>> #define RTE_MBUF_TO_BADDR(mb)       (((struct rte_mbuf *)(mb)) + 1)
>>
>> These macros are used for attaching/detaching mbuf's to each other. This
>> is the way the code retrieves the direct buffer from an indirect one,
>> and vica versa. I think if we want to keep the metadata feature (which I
>> guess is quite important), we need to add a pointer to rte_mbuf, which
>> helps the direct and indirect structs to find each other. Something like:
>>
>>      struct rte_mbuf *attach;    /**< Points to the other buffer if this
>> one
>>                       is (in)direct. Otherwise NULL.  */
>>
>> What do you think?
>
> I've just sent a patch that should fix this issue.
> http://dpdk.org/ml/archives/dev/2015-March/015722.html
>
> Let me know if you have any comment on it.

I have some comments for the first patch:

> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index c3fcb80..050f3ac 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
I've sent in a separate patch for this file, I think it's just easier to 
ditch the old copy-pasted code, see "[PATCH] examples/vhost: use library 
routines instead of local copies"

> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 17ba791..4ced6d3 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -268,7 +268,7 @@ struct rte_mbuf {
>  	uint16_t data_len;        /**< Amount of data in segment buffer. */
>  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
>  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order) */
> -	uint16_t reserved;
> +	uint16_t priv_size;       /**< size of the application private data */
>  	union {
>  		uint32_t rss;     /**< RSS hash result if RSS enabled */
>  		struct {
> @@ -320,15 +320,38 @@ struct rte_mbuf {
>  } __rte_cache_aligned;
>
>  /**
> - * Given the buf_addr returns the pointer to corresponding mbuf.
> + * Return the mbuf owning the given data buffer address.
> + *
> + * @param mi
> + *   The pointer to the indirect mbuf.
> + * @param buffer_addr
> + *   The address of the data buffer of the direct mbuf.
You don't need this parameter, it's mi->buf_addr.

> @@ -744,9 +767,11 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md)
>  static inline void rte_pktmbuf_detach(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_len = mp->elt_size - sizeof(*m);
I don't see any reason to keep buf and buf_len, just assign straight to 
m->buf_addr and *len.
Besides that, you need to deduct m->priv_size from buf_len.

> -	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m);
> +
> +	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof (*m) +
> +		m->priv_size;
>
>  	m->buf_addr = buf;
>  	m->buf_len = (uint16_t)buf_len;

The rest of the series looks good,

Reviewed-by: Zoltan Kiss <zoltan.kiss@linaro.org>

Re: [dpdk-dev] ovs-dpdk: placing the metadata

[Olivier MATZ]

Hi Zoltan,

On 03/25/2015 07:57 PM, Zoltan Kiss wrote:
> I have some comments for the first patch:
>
>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
>> index c3fcb80..050f3ac 100644
>> --- a/examples/vhost/main.c
>> +++ b/examples/vhost/main.c
> I've sent in a separate patch for this file, I think it's just easier to
> ditch the old copy-pasted code, see "[PATCH] examples/vhost: use library
> routines instead of local copies"

As Konstantin stated in the other thread, this modification is
wrong, sorry. I don't see why I changed this code instead of keeping
the initial logic. I'll fix that in the v2.

>>  /**
>> - * Given the buf_addr returns the pointer to corresponding mbuf.
>> + * Return the mbuf owning the given data buffer address.
>> + *
>> + * @param mi
>> + *   The pointer to the indirect mbuf.
>> + * @param buffer_addr
>> + *   The address of the data buffer of the direct mbuf.
> You don't need this parameter, it's mi->buf_addr.

That's correct. In this case, I propose to change the name of the
function in:
struct rte_mbuf *rte_mbuf_from_indirect(struct rte_mbuf *mi)

>> @@ -744,9 +767,11 @@ static inline void rte_pktmbuf_attach(struct
>> rte_mbuf *mi, struct rte_mbuf *md)
>>  static inline void rte_pktmbuf_detach(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_len = mp->elt_size - sizeof(*m);
> I don't see any reason to keep buf and buf_len, just assign straight to
> m->buf_addr and *len.
> Besides that, you need to deduct m->priv_size from buf_len.

Agree. I suggest something like:

static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
{
	const struct rte_mempool *mp = m->pool;
	void *buf = rte_mbuf_to_baddr(m);
	unsigned mhdr_size = (char *)buf - (char *)m;

	m->buf_addr = buf;
	m->buf_physaddr = rte_mempool_virt2phy(mp, m) + mhdr_size;
	m->buf_len = (uint16_t)(mp->elt_size - mhdr_size);
...


> The rest of the series looks good,
>
> Reviewed-by: Zoltan Kiss <zoltan.kiss@linaro.org>

Thank you for your review. I'll send a v2 today.

Regards,
Olivier