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 05:45:34PM +0300, Andrew Rybchenko wrote:
> On 04/25/2018 02:34 AM, Yongseok Koh wrote:
> > On Tue, Apr 24, 2018 at 09:15:38PM +0200, Olivier Matz wrote:
> > > On Tue, Apr 24, 2018 at 09:21:00PM +0300, Andrew Rybchenko wrote:
> > > > On 04/24/2018 07:02 PM, Olivier Matz wrote:
> > > > > > > +	m->ol_flags |= EXT_ATTACHED_MBUF;
> > > > > > > +	m->shinfo = shinfo;
> > > > > > > +
> > > > > > > +	rte_mbuf_ext_refcnt_set(shinfo, 1);
> > > > > > Why is assignment used here? Cannot we attach extbuf already attached to
> > > > > > other mbuf?
> > > > > In rte_pktmbuf_attach(), this is true. That's not illogical to
> > > > > keep the same approach here. Maybe an assert could be added?
> > Like I described in the doc, intention is to attach external buffer by
> > _attach_extbuf() for the first time and _attach() is just for additional mbuf
> > attachment. Will add an assert.
> 
> Not sure that I understand. How should the second chunk with shared shinfo
> of the huge buffer be attached to a new mbuf?

Okay, I think I know what you misunderstood. This patch itself has no
consideration about huge buffer. It is to simply attach an external buffer to an
mbuf. Slicing huge buffer and attaching multiple mbufs is one of use-cases of
this feature. Let me take a few examples.

rte_pktmbuf_attach_extbuf(m, buf, buf_iova, buf_len, NULL, fcb, fcb_arg);

                |<------ buf_len ------>|
 +----+         +----------------+------+
 | m  |-------->| ext buf        |shif  |
 |    |         |                |refc=1|
 +----+         +----------------+------+
                |<- m->buf_len ->|

rte_pktmbuf_attach(m1, m);

 +----+         +----------------+------+
 | m  |-------->| ext buf        |shif  |
 |    |         |                |refc=2|
 +----+         +----------------+------+
                ^
 +----+         |
 | m1 |---------+
 |    |
 +----+

rte_pktmbuf_attach_extbuf(m, buf, buf_iova, buf_len, shinfo, fcb, fcb_arg);

                                 |<------ buf_len ------>|
 +------+         +----+         +-----------------------+
 |shinfo|<--------| m  |-------->| ext buf               |
 |refc=1|         |    |         |                       |
 +------+         +----+         +-----------------------+
                                 |<----- m->buf_len ---->|

rte_pktmbuf_attach(m1, m);

 +------+         +----+         +-----------------------+
 |shinfo|<--------| m  |-------->| ext buf               |
 |refc=2|         |    |         |                       |
 +------+         +----+         +-----------------------+
     ^                           ^
     |            +----+         |
     +------------| m1 |---------+
                  |    |
                  +----+

> > > > Other option is to have shinfo per small buf plus reference counter
> > > > per huge buf (which is decremented when small buf reference counter
> > > > becomes zero and free callback is executed). I guess it is assumed above.
> > > > My fear is that it is too much reference counters:
> > > >   1. mbuf reference counter
> > > >   2. small buf reference counter
> > > >   3. huge buf reference counter
> > > > May be it is possible use (1) for (2) as well?
> > > I would prefer to have only 2 reference counters, one in the mbuf
> > > and one in the shinfo.
> > Good discussion. It should be a design decision by user.
> > 
> > In my use-case, it would be a good idea to make all the mbufs in a same chunk
> > point to the same shared info in the head of the chunk and reset the refcnt of
> > shinfo to the total number of slices in the chunk.
> > 
> > +--+----+----+--------------+----+--------------+---+- - -
> > |global |head|mbuf1 data    |head|mbuf2 data    |   |
> > | shinfo|room|              |room|              |   |
> > +--+----+----+--------------+----+--------------+---+- - -
> 
> I don't understand how it can be achieved using proposed API.
 
For the following use-case,
 +--+----+----+--------------+----+--------------+---+- - -
 |global |head|mbuf1 data    |head|mbuf2 data    |   |
 | shinfo|room|              |room|              |   |
 +--+----+----+--------------+----+--------------+---+- - -
 ^       |<---- buf_len ---->|
 |
 mem

User can do,

g_shinfo = (struct rte_mbuf_ext_shared_info *)mem;
buf = mem + sizeof (*g_shinfo);
buf_iova = get_iova(buf);
rte_pktmbuf_attach_extbuf(m1, buf, buf_iova, buf_len, g_shinfo, fcb, fcb_arg);
rte_mbuf_ext_refcnt_update(g_shinfo, 1);
rte_pktmbuf_reset_headroom(m1);
buf += buf_len;
buf_iova = get_iova(buf);
rte_pktmbuf_attach_extbuf(m2, buf, buf_iova, buf_len, g_shinfo, fcb, fcb_arg);
rte_mbuf_ext_refcnt_update(g_shinfo, 1);
rte_pktmbuf_reset_headroom(m2);


Does it make sense?

Thanks,
Yongseok