[dpdk-dev] [PATCH v3 1/2] mbuf: support attaching external buffer to mbuf

[Yongseok Koh]

On Mon, Apr 23, 2018 at 11:53:04AM +0000, Ananyev, Konstantin wrote:
[...]
> > @@ -693,9 +711,14 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
> >  #define RTE_MBUF_INDIRECT(mb)   ((mb)->ol_flags & IND_ATTACHED_MBUF)
> > 
> >  /**
> > + * Returns TRUE if given mbuf has external buffer, or FALSE otherwise.
> > + */
> > +#define RTE_MBUF_HAS_EXTBUF(mb) ((mb)->ol_flags & EXT_ATTACHED_MBUF)
> > +
> > +/**
> >   * Returns TRUE if given mbuf is direct, or FALSE otherwise.
> >   */
> > -#define RTE_MBUF_DIRECT(mb)     (!RTE_MBUF_INDIRECT(mb))
> > +#define RTE_MBUF_DIRECT(mb) (!RTE_MBUF_INDIRECT(mb) && !RTE_MBUF_HAS_EXTBUF(mb))
> 
> As a nit:
> RTE_MBUF_DIRECT(mb)  (((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)) == 0)

It was for better readability and I expected compiler did the same.
But, if you still want this way, I can change it.

[...]
> > +static inline char * __rte_experimental
> > +rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr,
> > +	uint16_t buf_len, rte_mbuf_extbuf_free_callback_t free_cb,
> > +	void *fcb_opaque)
> > +{
> > +	void *buf_end = RTE_PTR_ADD(buf_addr, buf_len);
> > +	struct rte_mbuf_ext_shared_info *shinfo;
> > +
> > +	shinfo = RTE_PTR_ALIGN_FLOOR(RTE_PTR_SUB(buf_end, sizeof(*shinfo)),
> > +			sizeof(uintptr_t));
> > +
> > +	if ((void *)shinfo <= buf_addr)
> > +		return NULL;
> > +
> > +	m->buf_addr = buf_addr;
> > +	m->buf_iova = rte_mempool_virt2iova(buf_addr);
> 
> 
> That wouldn't work for arbitrary extern buffer.
> Only for the one that is an element in some other mempool.
> For arbitrary external buffer - callee has to provide PA for it plus guarantee that
> it's VA would be locked down.
> From other side - if your intention is just to use only elements of other mempools -
> No need to have free_cb(). mempool_put should do.

Of course, I didn't mean that. That was a mistake. Please refer to my reply to
Olivier.

[...]
> >  /**
> > - * Detach an indirect packet mbuf.
> > + * @internal used by rte_pktmbuf_detach().
> > + *
> > + * Decrement the reference counter of the external buffer. When the
> > + * reference counter becomes 0, the buffer is freed by pre-registered
> > + * callback.
> > + */
> > +static inline void
> > +__rte_pktmbuf_free_extbuf(struct rte_mbuf *m)
> > +{
> > +	struct rte_mbuf_ext_shared_info *shinfo;
> > +
> > +	RTE_ASSERT(RTE_MBUF_HAS_EXTBUF(m));
> > +
> > +	shinfo = rte_mbuf_ext_shinfo(m);
> > +
> > +	if (rte_extbuf_refcnt_update(shinfo, -1) == 0)
> > +		shinfo->free_cb(m->buf_addr, shinfo->fcb_opaque);
> 
> 
> I understand the reason but extra function call for each external mbuf - seems quite expensive.
> Wonder is it possible to group them somehow and amortize the cost?

Good point. I thought about it today.

Comparing to the regular mbuf, maybe three differences. a) free function isn't
inlined but a real branch. b) no help from core local cache like mempool's c) no
free_bulk func like rte_mempool_put_bulk(). But these look quite costly and
complicated for the external buffer attachment.

For example, to free it in bulk, external buffers should be grouped as the
buffers would have different callback functions. To do that, I have to make an
API to pre-register an external buffer group to prepare resources for the bulk
free. Then, buffers can't be anonymous anymore but have to be registered in
advance. If so, it would be better to use existing APIs, especially when a user
wants high throughput...

Let me know if you have better idea to implement it. Then, I'll gladly take
that. Or, we can push any improvement patch in the next releases.


Thanks,
Yongseok