Re: ovs-dpdk: placing the metadata

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

Regards,
Olivier