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/27/2015 10:07 AM, Olivier MATZ wrote:
>> I think that to support ability to setup priv_size on a mempool basis,
>> and reserve private space between struct rte_mbuf and rte_mbuf. buf_addr,
>> we need to: 
>>
>> 1. Store priv_size both inside the mempool and inside the mbuf.
>>
>> 2. rte_pktmbuf_attach() should change the value of priv_size to the priv_size of the direct mbuf we are going to attach to:
>> rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md) {... mi->priv_size = md->priv_size; ...}
>>
>> 3. rte_pktmbuf_detach() should restore original value of mbuf's priv_size:
>> rte_pktmbuf_detach(struct rte_mbuf *m) 
>> {
>>   ...
>>    m->priv_size = rte_mempool_get_privsize(m->pool);
>>    m->buf _addr= rte_mbuf_to_baddr(m);
>>    ...
>> }
>>
>> Also I think we need to provide a way to specify priv_size for all mbufs of the mepool at init time:
>> - either force people to specify it at rte_mempool_create() time (probably use init_arg for that),
>> - or provide separate function that could be called straight after rte_mempool_create() , that
>> would setup priv_size for the  pool and for all its mbufs.
>> - or some sort of combination of these 2 approaches - introduce a wrapper function 
>> (rte_mbuf_pool_create() or something) that would take priv_size as parameter, 
>> create a new mempool and then setup priv_size.

I though a bit more about this solution, and I realized that doing
mi->priv_size = md->priv_size in rte_pktmbuf_attach() is probably not
a good idea, as there is no garantee that the size of mi is large enough
to store the priv of md.

Having the same priv_size for mi and md is maybe a good constraint.
I can add this in the API comments and assertions in the code to
check this condition, what do you think?


> Introducing rte_mbuf_pool_create() seems a good idea to me, it
> would hide 'rte_pktmbuf_pool_private' from the user and force
> to initialize all the required fields (mbuf_data_room_size only
> today, and maybe mbuf_priv_size).
> 
> The API would be:
> 
> struct rte_mempool *
> rte_mbuf_pool_create(const char *name, unsigned n, unsigned elt_size,
> 	unsigned cache_size, size_t mbuf_priv_size,
> 	rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
> 	int socket_id, unsigned flags)
> 
> I can give it a try and send a patch for this.

About this, it is not required anymore for this patch series if we
agree with my comment above.

I'll send a separate patch for that. It's probably a good occasion
to get rid of the pointer casted into an integer for
mbuf_data_room_size.

Regards,
Olivier