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

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

Hi,

On 04/01/2015 03:48 PM, 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?

Yes, indeed that would be a significant change for the applications,
which is probably not what we want, except if we can keep backward
compatibility. Maybe it's possible. That's something to keep in mind
if I send a patch series that introduce a new rte_mbuf_pool_create()
function.

Regards,
Olivier