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 at linaro.org>
