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

[Olivier MATZ <olivier.matz@6wind.com>]

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().

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


Regards
Olivier