[dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when application uses private mbuf data

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, March 31, 2015 8:01 PM
> To: Ananyev, Konstantin
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when application uses private mbuf data
> 
> Hi Konstantin,
> 
> On 03/31/2015 01:17 AM, Ananyev, Konstantin wrote:
> >>>> With this solution, there are 2 options:
> >>>> - no mempool modification, so each application/driver has to add
> >>>>   priv_size bytes to the object to get the mbuf pointer. This does not
> >>>>   look feasible.
> >>>> - change the mempool to support private area before each object. I
> >>>>   think mempool API is already quite complex, and I think it's not
> >>>>   the role of the mempool library to provide such features.
> >>>
> >>>
> >>> In fact, I think the changes would be minimal.
> >>> All we have to do, is to make changes in rte_pktmbuf_init():
> >>>
> >>> void
> >>> rte_pktmbuf_init(struct rte_mempool *mp,
> >>>                  __attribute__((unused)) void *opaque_arg,
> >>>                  void *_m,
> >>>                  __attribute__((unused)) unsigned i)
> >>> {
> >>>
> >>>      //extract priv_size from mempool   (discussed above).
> >>>       uint16_t priv_size = rte_mbufpool_get_priv_size(mp);
> >>>
> >>>       struct rte_mbuf *m = _m + priv_size;
> >>>       uint32_t buf_len = mp->elt_size - sizeof(struct rte_mbuf) - priv_size;
> >>>
> >>> ....
> >>>
> >>>
> >>> With that way priv_size inside mbuf would always be the size its own private space,
> >>> for both direct and indirect mbus., so we don't require to set priv_size at attach/detach.
> >>> Again RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() would just work without any modifications.
> >>
> >> I'm not sure I'm getting it. The argument '_m' of your
> >> rte_pktmbuf_init() is the pointer to the priv data, right?
> >> So it means that the mbuf is located priv_size bytes after.
> >>
> >> The rte_pktmbuf_init() function is called by mempool_create(),
> >> and the _m parameter is a pointer to a mempool object. So
> >> in my understanding, mempool_get() would not return a rte_mbuf
> >> but a pointer to the application private data.
> >
> > Ah my bad, forgot that mempool's obj_init() returns void now :(
> > To make this approach work also need to change it, so it return a pointer to the new object.
> > So, rte_pktmbuf_init() would return m and then in mempool_add_elem():
> >
> > if (obj_init)
> >                 obj = obj_init(mp, obj_init_arg, obj, obj_idx);
> >
> > rte_ring_sp_enqueue(mp->ring, obj);
> 
> Yes, but modififying mempool is what I wanted to avoid for several
> reasons. First, I think that changing the mempool_create() API here
> (actually the obj_init prototype) would impact existing applications.
> 
> Also, I'm afraid that storing a different address than the start
> address of the object would have additional impacts. For instance,
> some data is supposed to be stored before the object, see
> __mempool_from_obj() or mempool_obj_audit().

mempool_obj_audit() should be ok, I think, but yes -
rte_mempool_from_obj() would change the behaviour and
can't be used by mempool_obj_audit() anymore.

Ok, I am convinced - let's stick with private space between rte_mbuf and buf_adddr, as you suggested.  

> 
> Finally, I believe that mempool is not the proper place to do
> modifications that are only needed for mbufs. If we really want
> to implement mbuf private data in that way, maybe it would be
> better to add a new layer above mempool (a mbuf pool layer).

Not that I am against it, but seems like even more massive change -
every application would need to be changed to use rte_mbufpool_create(), no?

Konstantin

> 
> 
> Regards
> Olivier