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