[dpdk-dev] [RFC 0/7] changing mbuf pool handler
On 5/10/2016 12:49 PM, Hemant Agrawal wrote:
> Hi Olivier,
>
>> -Original Message-
>> From: Hunt, David [mailto:david.hunt at intel.com]
>> Hi Olivier,
>>
>>
>> On 3/10/2016 4:49 PM, Olivier Matz wrote:
>>> Hi Hemant,
>>>
>>> Thank you for your feedback.
>>>
>>> On 09/22/2016 01:52 PM, Hemant Agrawal wrote:
Hi Olivier
On 9/19/2016 7:12 PM, Olivier Matz wrote:
> Hello,
>
> Following discussion from [1] ("usages issue with external mempool").
>
> This is a tentative to make the mempool_ops feature introduced by
> David Hunt [2] more widely used by applications.
>
> It applies on top of a minor fix in mbuf lib [3].
>
> To sumarize the needs (please comment if I did not got it properly):
>
> - new hw-assisted mempool handlers will soon be introduced
> - to make use of it, the new mempool API [4]
>> (rte_mempool_create_empty,
> rte_mempool_populate, ...) has to be used
> - the legacy mempool API (rte_mempool_create) does not allow to
>> change
> the mempool ops. The default is "ring_p_c" depending on
> flags.
> - the mbuf helper (rte_pktmbuf_pool_create) does not allow to change
> them either, and the default is RTE_MBUF_DEFAULT_MEMPOOL_OPS
> ("ring_mp_mc")
> - today, most (if not all) applications and examples use either
> rte_pktmbuf_pool_create or rte_mempool_create to create the mbuf
> pool, making it difficult to take advantage of this feature with
> existing apps.
>
> My initial idea was to deprecate both rte_pktmbuf_pool_create() and
> rte_mempool_create(), forcing the applications to use the new API,
> which is more flexible. But after digging a bit, it appeared that
> rte_mempool_create() is widely used, and not only for mbufs.
> Deprecating it would have a big impact on applications, and
> replacing it with the new API would be overkill in many use-cases.
I agree with the proposal.
> So I finally tried the following approach (inspired from a
> suggestion Jerin [5]):
>
> - add a new mempool_ops parameter to rte_pktmbuf_pool_create().
>> This
> unfortunatelly breaks the API, but I implemented an ABI compat layer.
> If the patch is accepted, we could discuss how to announce/schedule
> the API change.
> - update the applications and documentation to prefer
> rte_pktmbuf_pool_create() as much as possible
> - update most used examples (testpmd, l2fwd, l3fwd) to add a new
>> command
> line argument to select the mempool handler
>
> I hope the external applications would then switch to
> rte_pktmbuf_pool_create(), since it supports most of the use-cases
> (even priv_size != 0, since we can call rte_mempool_obj_iter() after) .
>
I will still prefer if you can add the "rte_mempool_obj_cb_t *obj_cb,
void *obj_cb_arg" into "rte_pktmbuf_pool_create". This single
consolidated wrapper will almost make it certain that applications
will not try to use rte_mempool_create for packet buffers.
>>> The patch changes the example applications. I'm not sure I understand
>>> why adding these arguments would force application to not use
>>> rte_mempool_create() for packet buffers. Do you have a application in
>> mind?
>>> For the mempool_ops parameter, we must pass it at init because we need
>>> to know the mempool handler before populating the pool. For object
>>> initialization, it can be done after, so I thought it was better to
>>> reduce the number of arguments to avoid to fall in the
>>> mempool_create() syndrom :)
>> I also agree with the proposal. Looks cleaner.
>>
>> I would lean to the side of keeping the parameters to the minimum, i.e.
>> not adding *obj_cb and *obj_cb_arg into rte_pktmbuf_pool_create.
>> Developers always have the option of going with rte_mempool_create if they
>> need more fine-grained control.
> [Hemant] The implementations with hw offloaded mempools don't want developer
> using *rte_mempool_create* for packet buffer pools.
> This API does not work for hw offloaded mempool.
>
> Also, *rte_mempool_create_empty* - may not be convenient for many
> application, as it requires calling 4+ APIs.
>
> Olivier is not in favor of deprecating the *rte_mempool_create*. I agree
> with concerns raised by him.
>
> Essentially, I was suggesting to upgrade * rte_pktmbuf_pool_create* to be
> like *rte_mempool_create* for packet buffers exclusively.
>
> This will provide a clear segregation for API usages w.r.t the packet buffer
> pool vs all other type of mempools.
Yes, it does sound like we need those extra parameters on
rte_pktmbuf_pool_create.
Regards,
Dave.
[dpdk-dev] [RFC 0/7] changing mbuf pool handler
Hi Olivier,
> -Original Message-
> From: Hunt, David [mailto:david.hunt at intel.com]
> Hi Olivier,
>
>
> On 3/10/2016 4:49 PM, Olivier Matz wrote:
> > Hi Hemant,
> >
> > Thank you for your feedback.
> >
> > On 09/22/2016 01:52 PM, Hemant Agrawal wrote:
> >> Hi Olivier
> >>
> >> On 9/19/2016 7:12 PM, Olivier Matz wrote:
> >>> Hello,
> >>>
> >>> Following discussion from [1] ("usages issue with external mempool").
> >>>
> >>> This is a tentative to make the mempool_ops feature introduced by
> >>> David Hunt [2] more widely used by applications.
> >>>
> >>> It applies on top of a minor fix in mbuf lib [3].
> >>>
> >>> To sumarize the needs (please comment if I did not got it properly):
> >>>
> >>> - new hw-assisted mempool handlers will soon be introduced
> >>> - to make use of it, the new mempool API [4]
> (rte_mempool_create_empty,
> >>>rte_mempool_populate, ...) has to be used
> >>> - the legacy mempool API (rte_mempool_create) does not allow to
> change
> >>>the mempool ops. The default is "ring_p_c" depending on
> >>>flags.
> >>> - the mbuf helper (rte_pktmbuf_pool_create) does not allow to change
> >>>them either, and the default is RTE_MBUF_DEFAULT_MEMPOOL_OPS
> >>>("ring_mp_mc")
> >>> - today, most (if not all) applications and examples use either
> >>>rte_pktmbuf_pool_create or rte_mempool_create to create the mbuf
> >>>pool, making it difficult to take advantage of this feature with
> >>>existing apps.
> >>>
> >>> My initial idea was to deprecate both rte_pktmbuf_pool_create() and
> >>> rte_mempool_create(), forcing the applications to use the new API,
> >>> which is more flexible. But after digging a bit, it appeared that
> >>> rte_mempool_create() is widely used, and not only for mbufs.
> >>> Deprecating it would have a big impact on applications, and
> >>> replacing it with the new API would be overkill in many use-cases.
> >> I agree with the proposal.
> >>
> >>> So I finally tried the following approach (inspired from a
> >>> suggestion Jerin [5]):
> >>>
> >>> - add a new mempool_ops parameter to rte_pktmbuf_pool_create().
> This
> >>>unfortunatelly breaks the API, but I implemented an ABI compat layer.
> >>>If the patch is accepted, we could discuss how to announce/schedule
> >>>the API change.
> >>> - update the applications and documentation to prefer
> >>>rte_pktmbuf_pool_create() as much as possible
> >>> - update most used examples (testpmd, l2fwd, l3fwd) to add a new
> command
> >>>line argument to select the mempool handler
> >>>
> >>> I hope the external applications would then switch to
> >>> rte_pktmbuf_pool_create(), since it supports most of the use-cases
> >>> (even priv_size != 0, since we can call rte_mempool_obj_iter() after) .
> >>>
> >> I will still prefer if you can add the "rte_mempool_obj_cb_t *obj_cb,
> >> void *obj_cb_arg" into "rte_pktmbuf_pool_create". This single
> >> consolidated wrapper will almost make it certain that applications
> >> will not try to use rte_mempool_create for packet buffers.
> > The patch changes the example applications. I'm not sure I understand
> > why adding these arguments would force application to not use
> > rte_mempool_create() for packet buffers. Do you have a application in
> mind?
> >
> > For the mempool_ops parameter, we must pass it at init because we need
> > to know the mempool handler before populating the pool. For object
> > initialization, it can be done after, so I thought it was better to
> > reduce the number of arguments to avoid to fall in the
> > mempool_create() syndrom :)
>
> I also agree with the proposal. Looks cleaner.
>
> I would lean to the side of keeping the parameters to the minimum, i.e.
> not adding *obj_cb and *obj_cb_arg into rte_pktmbuf_pool_create.
> Developers always have the option of going with rte_mempool_create if they
> need more fine-grained control.
[Hemant] The implementations with hw offloaded mempools don't want developer
using *rte_mempool_create* for packet buffer pools.
This API does not work for hw offloaded mempool.
Also, *rte_mempool_create_empty* - may not be convenient for many application,
as it requires calling 4+ APIs.
Olivier is not in favor of deprecating the *rte_mempool_create*. I agree with
concerns raised by him.
Essentially, I was suggesting to upgrade * rte_pktmbuf_pool_create* to be like
*rte_mempool_create* for packet buffers exclusively.
This will provide a clear segregation for API usages w.r.t the packet buffer
pool vs all other type of mempools.
Regards,
Hemant
>
> Regards,
> Dave.
>
> > Any other opinions?
> >
> > Regards,
> > Olivier
[dpdk-dev] [RFC 0/7] changing mbuf pool handler
Hi Olivier,
On 3/10/2016 4:49 PM, Olivier Matz wrote:
> Hi Hemant,
>
> Thank you for your feedback.
>
> On 09/22/2016 01:52 PM, Hemant Agrawal wrote:
>> Hi Olivier
>>
>> On 9/19/2016 7:12 PM, Olivier Matz wrote:
>>> Hello,
>>>
>>> Following discussion from [1] ("usages issue with external mempool").
>>>
>>> This is a tentative to make the mempool_ops feature introduced
>>> by David Hunt [2] more widely used by applications.
>>>
>>> It applies on top of a minor fix in mbuf lib [3].
>>>
>>> To sumarize the needs (please comment if I did not got it properly):
>>>
>>> - new hw-assisted mempool handlers will soon be introduced
>>> - to make use of it, the new mempool API [4] (rte_mempool_create_empty,
>>>rte_mempool_populate, ...) has to be used
>>> - the legacy mempool API (rte_mempool_create) does not allow to change
>>>the mempool ops. The default is "ring_p_c" depending on
>>>flags.
>>> - the mbuf helper (rte_pktmbuf_pool_create) does not allow to change
>>>them either, and the default is RTE_MBUF_DEFAULT_MEMPOOL_OPS
>>>("ring_mp_mc")
>>> - today, most (if not all) applications and examples use either
>>>rte_pktmbuf_pool_create or rte_mempool_create to create the mbuf
>>>pool, making it difficult to take advantage of this feature with
>>>existing apps.
>>>
>>> My initial idea was to deprecate both rte_pktmbuf_pool_create() and
>>> rte_mempool_create(), forcing the applications to use the new API, which
>>> is more flexible. But after digging a bit, it appeared that
>>> rte_mempool_create() is widely used, and not only for mbufs. Deprecating
>>> it would have a big impact on applications, and replacing it with the
>>> new API would be overkill in many use-cases.
>> I agree with the proposal.
>>
>>> So I finally tried the following approach (inspired from a suggestion
>>> Jerin [5]):
>>>
>>> - add a new mempool_ops parameter to rte_pktmbuf_pool_create(). This
>>>unfortunatelly breaks the API, but I implemented an ABI compat layer.
>>>If the patch is accepted, we could discuss how to announce/schedule
>>>the API change.
>>> - update the applications and documentation to prefer
>>>rte_pktmbuf_pool_create() as much as possible
>>> - update most used examples (testpmd, l2fwd, l3fwd) to add a new command
>>>line argument to select the mempool handler
>>>
>>> I hope the external applications would then switch to
>>> rte_pktmbuf_pool_create(), since it supports most of the use-cases (even
>>> priv_size != 0, since we can call rte_mempool_obj_iter() after) .
>>>
>> I will still prefer if you can add the "rte_mempool_obj_cb_t *obj_cb,
>> void *obj_cb_arg" into "rte_pktmbuf_pool_create". This single
>> consolidated wrapper will almost make it certain that applications will
>> not try to use rte_mempool_create for packet buffers.
> The patch changes the example applications. I'm not sure I understand
> why adding these arguments would force application to not use
> rte_mempool_create() for packet buffers. Do you have a application in mind?
>
> For the mempool_ops parameter, we must pass it at init because we need
> to know the mempool handler before populating the pool. For object
> initialization, it can be done after, so I thought it was better to
> reduce the number of arguments to avoid to fall in the mempool_create()
> syndrom :)
I also agree with the proposal. Looks cleaner.
I would lean to the side of keeping the parameters to the minimum, i.e.
not adding *obj_cb and *obj_cb_arg into rte_pktmbuf_pool_create.
Developers always have the option of going with rte_mempool_create if
they need more fine-grained control.
Regards,
Dave.
> Any other opinions?
>
> Regards,
> Olivier
[dpdk-dev] [RFC 0/7] changing mbuf pool handler
Hi Hemant,
Thank you for your feedback.
On 09/22/2016 01:52 PM, Hemant Agrawal wrote:
> Hi Olivier
>
> On 9/19/2016 7:12 PM, Olivier Matz wrote:
>> Hello,
>>
>> Following discussion from [1] ("usages issue with external mempool").
>>
>> This is a tentative to make the mempool_ops feature introduced
>> by David Hunt [2] more widely used by applications.
>>
>> It applies on top of a minor fix in mbuf lib [3].
>>
>> To sumarize the needs (please comment if I did not got it properly):
>>
>> - new hw-assisted mempool handlers will soon be introduced
>> - to make use of it, the new mempool API [4] (rte_mempool_create_empty,
>> rte_mempool_populate, ...) has to be used
>> - the legacy mempool API (rte_mempool_create) does not allow to change
>> the mempool ops. The default is "ring_p_c" depending on
>> flags.
>> - the mbuf helper (rte_pktmbuf_pool_create) does not allow to change
>> them either, and the default is RTE_MBUF_DEFAULT_MEMPOOL_OPS
>> ("ring_mp_mc")
>> - today, most (if not all) applications and examples use either
>> rte_pktmbuf_pool_create or rte_mempool_create to create the mbuf
>> pool, making it difficult to take advantage of this feature with
>> existing apps.
>>
>> My initial idea was to deprecate both rte_pktmbuf_pool_create() and
>> rte_mempool_create(), forcing the applications to use the new API, which
>> is more flexible. But after digging a bit, it appeared that
>> rte_mempool_create() is widely used, and not only for mbufs. Deprecating
>> it would have a big impact on applications, and replacing it with the
>> new API would be overkill in many use-cases.
>
> I agree with the proposal.
>
>>
>> So I finally tried the following approach (inspired from a suggestion
>> Jerin [5]):
>>
>> - add a new mempool_ops parameter to rte_pktmbuf_pool_create(). This
>> unfortunatelly breaks the API, but I implemented an ABI compat layer.
>> If the patch is accepted, we could discuss how to announce/schedule
>> the API change.
>> - update the applications and documentation to prefer
>> rte_pktmbuf_pool_create() as much as possible
>> - update most used examples (testpmd, l2fwd, l3fwd) to add a new command
>> line argument to select the mempool handler
>>
>> I hope the external applications would then switch to
>> rte_pktmbuf_pool_create(), since it supports most of the use-cases (even
>> priv_size != 0, since we can call rte_mempool_obj_iter() after) .
>>
>
> I will still prefer if you can add the "rte_mempool_obj_cb_t *obj_cb,
> void *obj_cb_arg" into "rte_pktmbuf_pool_create". This single
> consolidated wrapper will almost make it certain that applications will
> not try to use rte_mempool_create for packet buffers.
The patch changes the example applications. I'm not sure I understand
why adding these arguments would force application to not use
rte_mempool_create() for packet buffers. Do you have a application in mind?
For the mempool_ops parameter, we must pass it at init because we need
to know the mempool handler before populating the pool. For object
initialization, it can be done after, so I thought it was better to
reduce the number of arguments to avoid to fall in the mempool_create()
syndrom :)
Any other opinions?
Regards,
Olivier
