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

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Apr 25 20:05:12 CEST 2018



> -----Original Message-----
> From: Yongseok Koh [mailto:yskoh at mellanox.com]
> Sent: Wednesday, April 25, 2018 5:44 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>; olivier.matz at 6wind.com; dev at dpdk.org;
> adrien.mazarguil at 6wind.com; nelio.laranjeiro at 6wind.com
> Subject: Re: [PATCH v3 1/2] mbuf: support attaching external buffer to mbuf
> 
> On Wed, Apr 25, 2018 at 01:16:38PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > >
> > > 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.
> >
> > I know compilers are quite smart these days, but you never know for sure,
> > so yes, I think better to do that explicitly.
> 
> Okay.
> 
> > > [...]
> > > > >  /**
> > > > > - * 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.
> >
> > I don't have any extra-smart thoughts here.
> > One option I thought about - was to introduce group of external buffers with
> > common free routine (I think o mentioned it already).
> > Second - hide all that external buffer management inside mempool,
> > i.e. if user wants to use external buffers he create a mempool
> > (with rte_mbuf_ext_shared_info as elements?), then attach external buffer to shinfo
> > and call mbuf_attach_external(mbuf, shinfo).
> > Though for free we can just call mempool_put(shinfo) and let particular implementation
> > decide when/how call free_cb(), etc.
> I don't want to restrict external buffer to mempool object. Especially for
> storage users, they want to use **any** buffer, even coming outside of DPDK.

I am not talking about the case when external buffer can be allocated from mempool.
I am talking about the implementation where shinfo is a a mempool element.
So to bring extrernal buffer into DPDK - users get a shinfo (from mempool) and attach it
to external buffer.
When no one needs that external buffer any more (shinfo.refcnt == 0) 
mempool_put() is invoked for shinfo.
Inside put() we can either call free_cb() or keep extrenal buffer for further usage.
Anyway just a thought.
Konstantin

> 
> However, will open a follow-up discussion for this in the next release window
> probably with more measurement data.
> Thank you for suggestions.
> 
> Yongseok


More information about the dev mailing list