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

[Yongseok Koh]

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:
> > > Hi Andrew, Yongseok,
> > > 
> > > On Tue, Apr 24, 2018 at 03:28:33PM +0300, Andrew Rybchenko wrote:
> > > > On 04/24/2018 04:38 AM, Yongseok Koh wrote:
[...]
> > > > > +	m->buf_addr = buf_addr;
> > > > > +	m->buf_iova = buf_iova;
> > > > > +
> > > > > +	if (shinfo == NULL) {
> > > > > +		shinfo = RTE_PTR_ALIGN_FLOOR(RTE_PTR_SUB(buf_end,
> > > > > +					sizeof(*shinfo)), sizeof(uintptr_t));
> > > > > +		if ((void *)shinfo <= buf_addr)
> > > > > +			return NULL;
> > > > > +
> > > > > +		m->buf_len = RTE_PTR_DIFF(shinfo, buf_addr);
> > > > > +	} else {
> > > > > +		m->buf_len = buf_len;
> > > > > +	}
> > > > > +
> > > > > +	m->data_len = 0;
> > > > > +
> > > > > +	rte_pktmbuf_reset_headroom(m);
> > > > I would suggest to make data_off one more parameter.
> > > > If I have a buffer with data which I'd like to attach to an mbuf, I'd like
> > > > to control data_off.
> > > Another option is to set the headroom to 0.
> > > Because the after attaching the mbuf to an external buffer, we will
> > > still require to set the length.
> > > 
> > > A user can do something like this:
> > > 
> > > 	rte_pktmbuf_attach_extbuf(m, buf_va, buf_iova, buf_len, shinfo,
> > > 		free_cb, free_cb_arg);
> > > 	rte_pktmbuf_append(m, data_len + headroom);
> > > 	rte_pktmbuf_adj(m, headroom);

I'd take this option. Will make the change and document it.

> > > > > +	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.

> > > > May be shinfo should be initialized only if it is not provided (shinfo ==
> > > > NULL on input)?
> > > I don't get why, can you explain please?
> > 
> > May be I misunderstand how it should look like when one huge buffer
> > is partitioned. I thought that it should be only one shinfo per huge buffer
> > to control when it is not used any more by any mbufs with extbuf.
> 
> OK I got it.
> 
> I think both approach could make sense:
> - one shinfo per huge buffer
> - or one shinfo per mbuf, and use the callback to manage another refcnt
>   (like what Yongseok described)
> 
> So I agree with your proposal, shinfo should be initialized by
> the caller if it is != NULL, else it can be initialized by
> rte_pktmbuf_attach_extbuf().

Also agreed. Will change.

> > 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|              |   |
+--+----+----+--------------+----+--------------+---+- - -


Thanks,
Yongseok