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

[Yongseok Koh <yskoh@mellanox.com>]

On Wed, Apr 25, 2018 at 10:00:10PM +0200, Olivier Matz wrote:
> On Wed, Apr 25, 2018 at 09:19:32AM +0000, Yongseok Koh wrote:
> > > On Apr 25, 2018, at 2:08 AM, Yongseok Koh <yskoh@mellanox.com> wrote:
> > >> On Apr 25, 2018, at 1:28 AM, Olivier Matz <olivier.matz@6wind.com> wrote:
> > >> 
> > >> Hi Yongseok,
> > >> 
> > >> On Tue, Apr 24, 2018 at 06:02:44PM +0200, Olivier Matz wrote:
> > >>>>> @@ -688,14 +704,33 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
> > >>>>> }
> > >>>>> /**
> > >>>>> + * Returns TRUE if given mbuf is cloned by mbuf indirection, or FALSE
> > >>>>> + * otherwise.
> > >>>>> + *
> > >>>>> + * If a mbuf has its data in another mbuf and references it by mbuf
> > >>>>> + * indirection, this mbuf can be defined as a cloned mbuf.
> > >>>>> + */
> > >>>>> +#define RTE_MBUF_CLONED(mb)     ((mb)->ol_flags & IND_ATTACHED_MBUF)
> > >>>>> +
> > >>>>> +/**
> > >>>>>  * Returns TRUE if given mbuf is indirect, or FALSE otherwise.
> > >>>>>  */
> > >>>>> -#define RTE_MBUF_INDIRECT(mb)   ((mb)->ol_flags & IND_ATTACHED_MBUF)
> > >>>>> +#define RTE_MBUF_INDIRECT(mb)   RTE_MBUF_CLONED(mb)
> > >>>> 
> > >>>> It is still confusing that INDIRECT != !DIRECT.
> > >>>> May be we have no good options right now, but I'd suggest to at least
> > >>>> deprecate
> > >>>> RTE_MBUF_INDIRECT() and completely remove it in the next release.
> > >>> 
> > >>> Agree. I may have missed something, but is my previous suggestion
> > >>> not doable?
> > >>> 
> > >>> - direct = embeds its own data      (and indirect = !direct)
> > >>> - clone (or another name) = data is another mbuf
> > >>> - extbuf = data is in an external buffer
> > >> 
> > >> Any comment about this option?
> > > 
> > > I liked your idea, so I defined RTE_MBUF_CLONED() and wanted to deprecate
> > > RTE_MBUF_INDIRECT() in the coming release. But RTE_MBUF_DIRECT() can't be
> > > (!RTE_MBUF_INDIRECT()) because it will logically include RTE_MBUF_HAS_EXTBUF().
> > > I'm not sure I understand you correctly.
> > > 
> > > Can you please give me more guidelines so that I can take you idea?
> > 
> > Maybe, did you mean the following? Looks like doable but RTE_MBUF_DIRECT()
> > can't logically mean 'mbuf embeds its own data', right?
> > 
> > #define RTE_MBUF_INDIRECT(mb)   ((mb)->ol_flags & IND_ATTACHED_MBUF)
> > #define RTE_MBUF_DIRECT(mb)     (!RTE_MBUF_INDIRECT(mb))
> > 
> > #define RTE_MBUF_HAS_EXTBUF(mb) ((mb)->ol_flags & EXT_ATTACHED_MBUF)
> > #define RTE_MBUF_CLONED(mb)     ((mb)->ol_flags & IND_ATTACHED_MBUF)
> 
> I was thinking about something like this:
> 
> #define RTE_MBUF_HAS_EXTBUF(mb) ((mb)->ol_flags & EXT_ATTACHED_MBUF)
> #define RTE_MBUF_CLONED(mb)     ((mb)->ol_flags & IND_ATTACHED_MBUF)
> 
> #define RTE_MBUF_DIRECT(mb)     (!RTE_MBUF_HAS_EXTBUF(mb) && !RTE_MBUF_CLONED(mb))
> #define RTE_MBUF_INDIRECT(mb)   (!RTE_MBUF_DIRECT(mb))

Then, indirect should mean either having IND_ATTACHED_MBUF or having
EXT_ATTACHED_MBUF and it sounds weird. In a situation where EXT_ATTACHED_MBUF
(experimental) is never set, your definition of indirect will be same as now. No
breakage either.  Although your idea is logically same as the current patch,
there seems to be semantic conflict.

Thanks,
Yongseok