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

[Zoltan Kiss <zoltan.kiss@linaro.org>]

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>