[dpdk-dev] 17.02 Roadmap
Hi Thomas, On 10/10/2016 9:42 PM, Thomas Monjalon wrote: > Thanks Tim for the interesting inputs. > Some of them may require a dedicated thread to continue the discussion > based on some preliminary specifications or drafts. > > 2016-10-10 16:13, O'Driscoll, Tim: >> Packet Distributor Enhancements: Enhancements will be made to the Packet >> Distributor library to improve performance: >> 1. Introduce burst functionality to allow batches of packets to be sent to >> workers. >> 2. Improve the performance of the flow/core affinity through the use of >> SSE/AVX instructions. > The packet distributor looks more and more like a DPDK application > at the same level of BESS, VPP, OVS, etc. Lets discuss this further. Would you see other libraries heading in this direction also (reorder, acl, hash, etc)? Do you think it would be an idea to add as an item of discussion for the technical steering group when we're all at Userspace next week? Regards, Dave.
[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_<s|m>p_<s|m>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,
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] [PATCH v2 1/2] eal/mempool: introduce check for external mempool availability
Hi Hemant,
On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> External offloaded mempool may not be available always. This check enables
> run time verification of the presence of external mempool before the
> mempool ops are installed.
>
> An application built with a specific external mempool may work fine
> on host. But, may not work on VM, specificaly if non-hw specific interfaces
> are used e.g. virtio-net.
>
> This is required to make sure that same application can work in all
> environment for a given hw platform.
>
> The solution proposed here is that rte_mempool_set_ops_byname should return
> specific error in case the current execution environment is not supporting
> the given mempool. Thereby allowing it to fallback on software mempool
> implementation e.g. "ring_mp_mc"
>
> This patch introduces new optional "pool_verify" as mempool ops function
> to check if external mempool instance is available or not.
I've a small suggestion around the naming of the new functions. It seems
to me that this patch is more about mempool handler 'support' than
'verification'.
Could I suggest a different name for this new function? I think maybe
"supported" may be more appropriate than "pool_verify". The return code
that's returned when a mempool handler is not available is -EOPNOTSUPP,
which is what suggested the word "supported" to me. I think that:
if (ops->supported)
return ops->supported(mp);
makes for slightly easier reading. The wrapper function would logically
then be called rte_mempool_ops_supported().
> Signed-off-by: Hemant Agrawal
> ---
> Changes in v2:
> * fixed the pool_verify copy in rte_mempool_register_ops
> ---
> lib/librte_mempool/rte_mempool.h | 21 +
> lib/librte_mempool/rte_mempool_ops.c | 19 +++
> lib/librte_mempool/rte_mempool_ring.c | 4
> lib/librte_mempool/rte_mempool_stack.c | 3 ++-
> 4 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_mempool/rte_mempool.h
> b/lib/librte_mempool/rte_mempool.h
> index 0243f9e..8599df1 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool
> *mp,
>*/
> typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp);
>
> +/**
> + * Return if the given external mempool is available for this instance.
> + * it is optional to implement for mempools
> + */
> +typedef int (*rte_mempool_pool_verify)(const struct rte_mempool *mp);
> +
> /** Structure defining mempool operations structure */
> struct rte_mempool_ops {
> char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
> @@ -395,6 +401,8 @@ struct rte_mempool_ops {
> rte_mempool_enqueue_t enqueue; /**< Enqueue an object. */
> rte_mempool_dequeue_t dequeue; /**< Dequeue an object. */
> rte_mempool_get_count get_count; /**< Get qty of available objs. */
> + rte_mempool_pool_verify pool_verify;
> + /**< Verify if external mempool is available for usages*/
> } __rte_cache_aligned;
>
> #define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
> @@ -516,6 +524,18 @@ void
> rte_mempool_ops_free(struct rte_mempool *mp);
>
> /**
> + * @internal wrapper to verify the external mempool availability
> + *
> + * @param mp
> + * Pointer to the memory pool.
> + * @return
> + * 0: Success; external mempool instance is available
> + * - <0: Error; external mempool instance is not available
> + */
> +int
> +rte_mempool_ops_pool_verify(const struct rte_mempool *mp);
> +
> +/**
>* Set the ops of a mempool.
>*
>* This can only be done on a mempool that is not populated, i.e. just after
> @@ -531,6 +551,7 @@ rte_mempool_ops_free(struct rte_mempool *mp);
>* - 0: Success; the mempool is now using the requested ops functions.
>* - -EINVAL - Invalid ops struct name provided.
>* - -EEXIST - mempool already has an ops struct assigned.
> + * - -EOPNOTSUPP - mempool instance not available.
>*/
> int
> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> diff --git a/lib/librte_mempool/rte_mempool_ops.c
> b/lib/librte_mempool/rte_mempool_ops.c
> index 5f24de2..c2e765f 100644
> --- a/lib/librte_mempool/rte_mempool_ops.c
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
> ops->enqueue = h->enqueue;
> ops->dequeue = h->dequeue;
> ops->get_count = h->get_count;
> + ops->pool_verify = h->pool_verify;
>
> rte_spinlock_unlock(_mempool_ops_table.sl);
>
> @@ -123,6 +124,18 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
> return ops->get_count(mp);
> }
>
> +/* wrapper to check if given external mempool is available for this
> instance.*/
> +int
> +rte_mempool_ops_pool_verify(const struct rte_mempool *mp)
> +{
> + struct
[dpdk-dev] [PATCH v2 2/2] mempool:pktmbuf pool default fallback for mempool ops error
Hi Hemant,
On 15/9/2016 6:13 PM, Hemant Agrawal wrote:
> In the rte_pktmbuf_pool_create, if the default external mempool is
> not available, the implementation can default to "ring_mp_mc", which
> is an software implementation.
>
> Signed-off-by: Hemant Agrawal
> ---
> lib/librte_mbuf/rte_mbuf.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 4846b89..4adb4f5 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -176,6 +176,11 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
>
> rte_errno = rte_mempool_set_ops_byname(mp,
> RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
> +
> + /* on error, try falling back to the software based default pool */
> + if (rte_errno == -EOPNOTSUPP)
> + rte_errno = rte_mempool_set_ops_byname(mp, "ring_mp_mc", NULL);
Should we log a warning message here saying that we're falling back to
the mp/mc handler?
> +
> if (rte_errno != 0) {
> RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> return NULL;
[dpdk-dev] [PATCH v3 13/15] ether: extract function eth_dev_get_driver_name
On 9/9/2016 9:43 AM, Shreyansh Jain wrote:
> Signed-off-by: Jan Viktorin
> Signed-off-by: Shreyansh Jain
> Signed-off-by: Hemant Agrawal
> ---
> lib/librte_ether/rte_ethdev.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 104ea4a..4fa65ca 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2568,6 +2568,17 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int
> op, void *data)
> return 0;
> }
>
> +static inline
> +const char *eth_dev_get_driver_name(const struct rte_eth_dev *dev)
> +{
> + if (dev->pci_dev) {
> + return dev->driver->pci_drv.driver.name;
> + }
> +
> + RTE_VERIFY(0);
Same comment as last patch, maybe add an rte_panic with more descriptive
error message.
> + return NULL;
> +}
> +
> const struct rte_memzone *
> rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char
> *ring_name,
>uint16_t queue_id, size_t size, unsigned align,
> @@ -2575,9 +2586,11 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev
> *dev, const char *ring_name,
> {
> char z_name[RTE_MEMZONE_NAMESIZE];
> const struct rte_memzone *mz;
> + const char *drv_name;
>
> + drv_name = eth_dev_get_driver_name(dev);
> snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> - dev->driver->pci_drv.driver.name, ring_name,
> + drv_name, ring_name,
>dev->data->port_id, queue_id);
>
> mz = rte_memzone_lookup(z_name);
[dpdk-dev] [PATCH v3 12/15] ether: extract function eth_dev_get_intr_handle
On 9/9/2016 9:43 AM, Shreyansh Jain wrote:
> We abstract access to the intr_handle here as we want to get
> it either from the pci_dev or soc_dev.
>
> Signed-off-by: Jan Viktorin
> Signed-off-by: Shreyansh Jain
> Signed-off-by: Hemant Agrawal
> ---
> lib/librte_ether/rte_ethdev.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index e9f5467..104ea4a 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2526,6 +2526,17 @@ _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> rte_spinlock_unlock(_eth_dev_cb_lock);
> }
>
> +static inline
> +struct rte_intr_handle *eth_dev_get_intr_handle(struct rte_eth_dev *dev)
> +{
> + if (dev->pci_dev) {
> + return >pci_dev->intr_handle;
> + }
> +
> + RTE_VERIFY(0);
Rather than RTE_VERIFY(0), might I suggest using rte_panic with a more
relevant error message?
> + return NULL;
> +}
> +
> int
> rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int op, void *data)
> {
> @@ -2538,7 +2549,7 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, int
> op, void *data)
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>
> dev = _eth_devices[port_id];
> - intr_handle = >pci_dev->intr_handle;
> + intr_handle = eth_dev_get_intr_handle(dev);
> if (!intr_handle->intr_vec) {
> RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
> return -EPERM;
> @@ -2598,7 +2609,7 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t
> queue_id,
> return -EINVAL;
> }
>
> - intr_handle = >pci_dev->intr_handle;
> + intr_handle = eth_dev_get_intr_handle(dev);
> if (!intr_handle->intr_vec) {
> RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n");
> return -EPERM;
[dpdk-dev] [PATCH v3 02/15] eal/soc: add rte_eal_soc_register/unregister logic
On 9/9/2016 9:43 AM, Shreyansh Jain wrote:
> Registeration of a SoC driver through a helper DRIVER_REGISTER_SOC
> (on the lines of DRIVER_REGISTER_PCI). soc_driver_list stores all the
> registered drivers.
>
> Test case has been introduced to verify the registration and
> deregistration.
>
> Signed-off-by: Jan Viktorin
> Signed-off-by: Shreyansh Jain
> Signed-off-by: Hemant Agrawal
> ---
> app/test/test_soc.c | 111
>
> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 4 +
> lib/librte_eal/common/eal_common_soc.c | 56
> lib/librte_eal/common/include/rte_soc.h | 26 ++
> lib/librte_eal/linuxapp/eal/Makefile| 1 +
> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 3 +
> 6 files changed, 201 insertions(+)
> create mode 100644 lib/librte_eal/common/eal_common_soc.c
>
> diff --git a/app/test/test_soc.c b/app/test/test_soc.c
> index 916a863..ac03e64 100644
> --- a/app/test/test_soc.c
> +++ b/app/test/test_soc.c
> @@ -75,6 +75,108 @@ static int test_compare_addr(void)
> free(a2.name);
> free(a1.name);
> free(a0.name);
> +
> + return 0;
> +}
> +
> +/**
> + * Empty PMD driver based on the SoC infra.
> + *
> + * The rte_soc_device is usually wrapped in some higher-level struct
> + * (eth_driver). We simulate such a wrapper with an anonymous struct here.
> + */
> +struct test_wrapper {
> + struct rte_soc_driver soc_drv;
> +};
> +
> +struct test_wrapper empty_pmd0 = {
> + .soc_drv = {
> + .driver = {
> + .name = "empty_pmd0"
> + },
> + },
> +};
> +
> +struct test_wrapper empty_pmd1 = {
> + .soc_drv = {
> + .driver = {
> + .name = "empty_pmd1"
> + },
> + },
> +};
> +
> +static int
> +count_registered_socdrvs(void)
> +{
> + int i;
> + struct rte_soc_driver *drv;
> +
> + i = 0;
> + TAILQ_FOREACH(drv, _driver_list, next)
> + i += 1;
> +
> + return i;
> +}
> +
> +static int
> +test_register_unregister(void)
> +{
> + struct rte_soc_driver *drv;
> + int count;
> +
> + rte_eal_soc_register(_pmd0.soc_drv);
> +
> + TEST_ASSERT(!TAILQ_EMPTY(_driver_list),
> + "No PMD is present but the empty_pmd0 should be there");
> + drv = TAILQ_FIRST(_driver_list);
> + TEST_ASSERT(!strcmp(drv->driver.name, "empty_pmd0"),
> + "The registered PMD is not empty_pmd0 but '%s'",
> + drv->driver.name);
> +
> + rte_eal_soc_register(_pmd1.soc_drv);
> +
> + count = count_registered_socdrvs();
> + TEST_ASSERT_EQUAL(count, 2, "Expected 2 PMDs but detected %d", count);
> +
> + rte_eal_soc_unregister(_pmd0.soc_drv);
> + count = count_registered_socdrvs();
> + TEST_ASSERT_EQUAL(count, 1, "Expected 1 PMDs but detected %d", count);
> +
> + rte_eal_soc_unregister(_pmd1.soc_drv);
> +
> + printf("%s has been successful\n", __func__);
> + return 0;
> +}
> +
> +/* save real devices and drivers until the tests finishes */
> +struct soc_driver_list real_soc_driver_list =
> + TAILQ_HEAD_INITIALIZER(real_soc_driver_list);
> +
> +static int test_soc_setup(void)
> +{
> + struct rte_soc_driver *drv;
> +
> + /* no real drivers for the test */
> + while (!TAILQ_EMPTY(_driver_list)) {
> + drv = TAILQ_FIRST(_driver_list);
> + rte_eal_soc_unregister(drv);
> + TAILQ_INSERT_TAIL(_soc_driver_list, drv, next);
> + }
> +
> + return 0;
> +}
> +
> +static int test_soc_cleanup(void)
> +{
> + struct rte_soc_driver *drv;
> +
> + /* bring back real drivers after the test */
> + while (!TAILQ_EMPTY(_soc_driver_list)) {
> + drv = TAILQ_FIRST(_soc_driver_list);
> + TAILQ_REMOVE(_soc_driver_list, drv, next);
> + rte_eal_soc_register(drv);
> + }
> +
> return 0;
> }
>
> @@ -84,6 +186,15 @@ test_soc(void)
> if (test_compare_addr())
> return -1;
>
> + if (test_soc_setup())
> + return -1;
> +
> + if (test_register_unregister())
> + return -1;
> +
> + if (test_soc_cleanup())
> + return -1;
> +
> return 0;
> }
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 7b3d409..cda8009 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -168,4 +168,8 @@ DPDK_16.11 {
>
> rte_eal_dev_attach;
> rte_eal_dev_detach;
> + soc_driver_list;
> + rte_eal_soc_register;
> + rte_eal_soc_unregister;
> +
> } DPDK_16.07;
> diff --git a/lib/librte_eal/common/eal_common_soc.c
> b/lib/librte_eal/common/eal_common_soc.c
> new file mode 100644
> index 000..56135ed
> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_soc.c
> @@ -0,0 +1,56 @@
> +/*-
> + * BSD LICENSE
[dpdk-dev] [PATCH v3 01/15] eal/soc: introduce very essential SoC infra definitions
Some small comments below:
On 9/9/2016 9:43 AM, Shreyansh Jain wrote:
> Define initial structures and functions for the SoC infrastructure.
> This patch supports only a very minimal functions for now.
> More features will be added in the following commits.
>
> Includes rte_device/rte_driver inheritance of
> rte_soc_device/rte_soc_driver.
>
> Signed-off-by: Jan Viktorin
> Signed-off-by: Shreyansh Jain
> Signed-off-by: Hemant Agrawal
> ---
> app/test/Makefile | 1 +
> app/test/test_soc.c | 90 +
> lib/librte_eal/common/Makefile | 2 +-
> lib/librte_eal/common/eal_private.h | 4 +
> lib/librte_eal/common/include/rte_soc.h | 138
>
> 5 files changed, 234 insertions(+), 1 deletion(-)
> create mode 100644 app/test/test_soc.c
> create mode 100644 lib/librte_eal/common/include/rte_soc.h
>
> diff --git a/app/test/Makefile b/app/test/Makefile
> index 611d77a..64b261d 100644
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -77,6 +77,7 @@ APP = test
> #
> SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) := commands.c
> SRCS-y += test.c
> +SRCS-y += test_soc.c
> SRCS-y += resource.c
> SRCS-y += test_resource.c
> test_resource.res: test_resource.c
> diff --git a/app/test/test_soc.c b/app/test/test_soc.c
> new file mode 100644
> index 000..916a863
> --- /dev/null
> +++ b/app/test/test_soc.c
> @@ -0,0 +1,90 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2016 RehiveTech. All rights reserved.
> + * All rights reserved.
Remove un-needed "All rights reserved"
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of RehiveTech nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include "test.h"
> +
> +static char *safe_strdup(const char *s)
> +{
> + char *c = strdup(s);
> +
> + if (c == NULL)
> + rte_panic("failed to strdup '%s'\n", s);
> +
> + return c;
> +}
> +
> +static int test_compare_addr(void)
> +{
> + struct rte_soc_addr a0;
> + struct rte_soc_addr a1;
> + struct rte_soc_addr a2;
> +
> + a0.name = safe_strdup("ethernet0");
> + a0.fdt_path = NULL;
> +
> + a1.name = safe_strdup("ethernet0");
> + a1.fdt_path = NULL;
> +
> + a2.name = safe_strdup("ethernet1");
> + a2.fdt_path = NULL;
> +
> + TEST_ASSERT(!rte_eal_compare_soc_addr(, ),
> + "Failed to compare two soc addresses that equal");
> + TEST_ASSERT(rte_eal_compare_soc_addr(, ),
> + "Failed to compare two soc addresses that differs");
> +
> + free(a2.name);
> + free(a1.name);
> + free(a0.name);
> + return 0;
> +}
> +
> +static int
> +test_soc(void)
> +{
> + if (test_compare_addr())
> + return -1;
> +
> + return 0;
> +}
> +
> +REGISTER_TEST_COMMAND(soc_autotest, test_soc);
> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
> index dfd64aa..b414008 100644
> --- a/lib/librte_eal/common/Makefile
> +++ b/lib/librte_eal/common/Makefile
> @@ -33,7 +33,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
>
> INC := rte_branch_prediction.h rte_common.h
> INC += rte_debug.h rte_eal.h rte_errno.h rte_launch.h rte_lcore.h
> -INC += rte_log.h rte_memory.h rte_memzone.h rte_pci.h
> +INC += rte_log.h rte_memory.h rte_memzone.h rte_soc.h rte_pci.h
> INC += rte_per_lcore.h
[dpdk-dev] [PATCH v3 00/15] Introduce SoC device/driver framework for EAL
Shreyansh, Jan, Hemant, On 9/9/2016 9:43 AM, Shreyansh Jain wrote: > Introduction: > = > > This patch set is direct derivative of Jan's original series [1],[2]. > > - As this deviates substantially from original series, if need be I can > post it as a separate patch rather than v2. Please suggest. > - Also, there are comments on original v1 ([4]) which are _not_ > incorporated in this series as they refer to section no more in new > version. > - This v3 version is based on the rte_driver/device patchset v9 [10]. > That series introduced device structures (rte_driver/rte_device) > generalizing devices into PCI, VDEV, XXX. For the purpose of this > patchset, XXX=>SOC. > > ---snip--- FYI, I've reviewed this patch set, and it looks to me like there's some very good work here. Each patch in the set builds nicely on the one before, and logically introduces the changes one by one. I've no functional suggestions as the implementation looks clean, but I've one or two tiny suggestions on headers and error messages. I'll add a reply to the relevant patches in the set. Also, there's one or two issues thrown up by checkpatch, but I suspect they're false positives, as I'm using the 4.6 kernel version. Regards, Dave
[dpdk-dev] [PATCH] ivshmem: fix for modified mempool struct
On 1/7/2016 5:26 PM, Ferruh Yigit wrote:
> struct rte_mempool changed its "ring" field to "pool_data"
>
> "ring" field is accessed by ivshmem library, and updated to "pool_data"
>
> This patch fixes the compile error:
>
> == Build lib/librte_ivshmem
>CC rte_ivshmem.o
> .../lib/librte_ivshmem/rte_ivshmem.c:
> In function 'add_mempool_to_metadata':
> .../lib/librte_ivshmem/rte_ivshmem.c:584:32:
> error: 'const struct rte_mempool' has no member named 'ring'
>return add_ring_to_metadata(mp->ring, config);
> ^~
>
> Fixes: 449c49b93a6b ("mempool: support handler operations")
> Signed-off-by: Ferruh Yigit
> ---
> lib/librte_ivshmem/rte_ivshmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_ivshmem/rte_ivshmem.c
> b/lib/librte_ivshmem/rte_ivshmem.c
> index 5c83920..c26edb6 100644
> --- a/lib/librte_ivshmem/rte_ivshmem.c
> +++ b/lib/librte_ivshmem/rte_ivshmem.c
> @@ -581,7 +581,7 @@ add_mempool_to_metadata(const struct rte_mempool *mp,
> }
>
> /* mempool consists of memzone and ring */
> - return add_ring_to_metadata(mp->ring, config);
> + return add_ring_to_metadata(mp->pool_data, config);
> }
>
> int
Acked-by: David Hunt
[dpdk-dev] [PATCH v4 2/2] test: migrate custom handler test to stack handler
On 30/6/2016 6:46 PM, Thomas Monjalon wrote: > 2016-06-30 18:36, Hunt, David: >> On 30/6/2016 10:45 AM, Thomas Monjalon wrote: >>> Hi David, >>> >>> There is a compilation error: >>> >>> app/test/test_mempool.c:598:6: error: >>> too few arguments to function ?test_mempool_basic? >>> >>> Please send a v5, thanks. >> Thomas, after the second commit patch-set is applied, there should only >> be 585 lines in test_mempool.c > I have 616 lines. > >> I cannot reproduce the issue you are seeing. I have pulled down the 2 >> patches from patchwork and it compiles and runs the autotest fine here. >> Could you please check on your side? > It is because of the patch from Lazaros. > Please rebase from master. OK, will do.
[dpdk-dev] [PATCH v4 2/2] test: migrate custom handler test to stack handler
On 30/6/2016 10:45 AM, Thomas Monjalon wrote: > Hi David, > > There is a compilation error: > > app/test/test_mempool.c:598:6: error: > too few arguments to function ?test_mempool_basic? > > Please send a v5, thanks. Thomas, after the second commit patch-set is applied, there should only be 585 lines in test_mempool.c I cannot reproduce the issue you are seeing. I have pulled down the 2 patches from patchwork and it compiles and runs the autotest fine here. Could you please check on your side? Regards, Dave.
[dpdk-dev] [PATCH] mbuf: replace c memcpy code semantics with optimized rte_memcpy
Hi Jerin,
I just ran a couple of tests on this patch on the latest master head on
a couple of machines. An older quad socket E5-4650 and a quad socket
E5-2699 v3
E5-4650:
I'm seeing a gain of 2% for un-cached tests and a gain of 9% on the
cached tests.
E5-2699 v3:
I'm seeing a loss of 0.1% for un-cached tests and a gain of 11% on the
cached tests.
This is purely the autotest comparison, I don't have traffic generator
results. But based on the above, I don't think there are any performance
issues with the patch.
Regards,
Dave.
On 24/5/2016 4:17 PM, Jerin Jacob wrote:
> On Tue, May 24, 2016 at 04:59:47PM +0200, Olivier Matz wrote:
>> Hi Jerin,
>>
>>
>> On 05/24/2016 04:50 PM, Jerin Jacob wrote:
>>> Signed-off-by: Jerin Jacob
>>> ---
>>> lib/librte_mempool/rte_mempool.h | 5 ++---
>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/librte_mempool/rte_mempool.h
>>> b/lib/librte_mempool/rte_mempool.h
>>> index ed2c110..ebe399a 100644
>>> --- a/lib/librte_mempool/rte_mempool.h
>>> +++ b/lib/librte_mempool/rte_mempool.h
>>> @@ -74,6 +74,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #ifdef __cplusplus
>>> extern "C" {
>>> @@ -917,7 +918,6 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const
>>> *obj_table,
>>> unsigned n, __rte_unused int is_mp)
>>> {
>>> struct rte_mempool_cache *cache;
>>> - uint32_t index;
>>> void **cache_objs;
>>> unsigned lcore_id = rte_lcore_id();
>>> uint32_t cache_size = mp->cache_size;
>>> @@ -946,8 +946,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const
>>> *obj_table,
>>> */
>>>
>>> /* Add elements back into the cache */
>>> - for (index = 0; index < n; ++index, obj_table++)
>>> - cache_objs[index] = *obj_table;
>>> + rte_memcpy(_objs[0], obj_table, sizeof(void *) * n);
>>>
>>> cache->len += n;
>>>
>>>
>> The commit title should be "mempool" instead of "mbuf".
> I will fix it.
>
>> Are you seeing some performance improvement by using rte_memcpy()?
> Yes, In some case, In default case, It was replaced with memcpy by the
> compiler itself(gcc 5.3). But when I tried external mempool manager patch and
> then performance dropped almost 800Kpps. Debugging further it turns out that
> external mempool managers unrelated change was knocking out the memcpy.
> explicit rte_memcpy brought back 500Kpps. Remaing 300Kpps drop is still
> unknown(In my test setup, packets are in the local cache, so it must be
> something do with __mempool_put_bulk text alignment change or similar.
>
> Anyone else observed performance drop with external poolmanager?
>
> Jerin
>
>> Regards
>> Olivier
[dpdk-dev] [PATCH v2 1/3] mempool: add stack (lifo) mempool handler
Hi Olivier, On 20/6/2016 9:17 AM, Olivier Matz wrote: > Hi David, > > On 06/17/2016 04:18 PM, Hunt, David wrote: >>> After reading it, I realize that it's nearly exactly the same code than >>> in "app/test: test external mempool handler". >>> http://patchwork.dpdk.org/dev/patchwork/patch/12896/ >>> >>> We should drop one of them. If this stack handler is really useful for >>> a performance use-case, it could go in librte_mempool. At the first >>> read, the code looks like a demo example : it uses a simple spinlock for >>> concurrent accesses to the common pool. Maybe the mempool cache hides >>> this cost, in this case we could also consider removing the use of the >>> rte_ring. >> While I agree that the code is similar, the handler in the test is a >> ring based handler, >> where as this patch adds an array based handler. > Not sure I'm getting what you are saying. Do you mean stack instead > of array? Yes, apologies, stack. > Actually, both are stacks when talking about bulks of objects. If we > consider each objects one by one, that's true the order will differ. > But as discussed in [1], the cache code already reverses the order of > objects when doing a mempool_get(). I'd say the reversing in cache code > is not really needed (only the order of object bulks should remain the > same). A rte_memcpy() looks to be faster, but it would require to do > some real-life tests to validate or unvalidate this theory. > > So to conclude, I still think both code in app/test and lib/mempool are > quite similar, and only one of them should be kept. > > [1] http://www.dpdk.org/ml/archives/dev/2016-May/039873.html OK, so we will probably remove the test app portion in the future is if is not needed, and if we apply the stack handler proposed in this patch set. >> I think that the case for leaving it in as a test for the standard >> handler as part of the >> previous mempool handler is valid, but maybe there is a case for >> removing it if >> we add the stack handler. Maybe a future patch? >> >>> Do you have some some performance numbers? Do you know if it scales >>> with the number of cores? >> For the mempool_perf_autotest, I'm seeing a 30% increase in performance >> for the >> local cache use-case for 1 - 36 cores (results vary within those tests >> between >> 10-45% gain, but with an average of 30% gain over all the tests.). >> >> However, for the tests with no local cache configured, throughput of the >> enqueue/dequeue >> drops by about 30%, with the 36 core yelding the largest drop of 40%. So >> this handler would >> not be recommended in no-cache applications. > Interesting, thanks. If you also have real-life (I mean network) > performance tests, I'd be interested too. I'm afraid don't currently have any real-life performance tests. > Ideally, we should have a documentation explaining in which cases a > handler or another should be used. However, if we don't know this > today, I'm not opposed to add this new handler in 16.07, and let people > do their tests and comment, then describe it properly for 16.11. > > What do you think? I agree. Add it in 16.07, and let people develop use cases for it, as well as possibly coming up with new handlers for 16.11. There's talk of hardware based handlers, I would also hope to see some of those contributed soon. Regards, David.
[dpdk-dev] [PATCH v14 1/3] mempool: support mempool handler operations
On 17/6/2016 3:35 PM, Jan Viktorin wrote:
> Hi David,
>
> still few nits... Do you like the upstreaming process? :) I hope finish this
> patchset soon. The major issues seem to be OK.
>
> [...]
>
>> +
>> +/**
>> + * @internal Get the mempool ops struct from its index.
>> + *
>> + * @param ops_index
>> + * The index of the ops struct in the ops struct table. It must be a valid
>> + * index: (0 <= idx < num_ops).
>> + * @return
>> + * The pointer to the ops struct in the table.
>> + */
>> +static inline struct rte_mempool_ops *
>> +rte_mempool_ops_get(int ops_index)
> What about to rename the ops_get to get_ops to not confuse
> with the ops wrappers? The thread on this topic has not
> been finished. I think, we can live with this name, it's
> just a bit weird...
>
> Olivier? Thomas?
I'll change it, if you think it's weird.
-rte_mempool_ops_get(int ops_index)
+rte_mempool_get_ops(int ops_index)
>> +{
>> +RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX);
> According to the doc comment:
>
> RTE_VERIFY(ops_index >= 0);
Fixed.
- RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX);
+ RTE_VERIFY((ops_index >= 0) && (ops_index <
RTE_MEMPOOL_MAX_OPS_IDX));
>> +
>> +return _mempool_ops_table.ops[ops_index];
>> +}
> [...]
>
>> +
>> +/**
>> + * @internal Wrapper for mempool_ops get callback.
> This comment is obsolete "get callback" vs. dequeue_bulk.
Done.
- * @internal Wrapper for mempool_ops get callback.
+ * @internal Wrapper for mempool_ops dequeue callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @param obj_table
>> + * Pointer to a table of void * pointers (objects).
>> + * @param n
>> + * Number of objects to get.
>> + * @return
>> + * - 0: Success; got n objects.
>> + * - <0: Error; code of get function.
> "get function" seems to be obsolete, too...
Done.
- * - <0: Error; code of get function.
+ * - <0: Error; code of dequeue function.
>> + */
>> +static inline int
>> +rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>> +void **obj_table, unsigned n)
>> +{
>> +struct rte_mempool_ops *ops;
>> +
>> +ops = rte_mempool_ops_get(mp->ops_index);
>> +return ops->dequeue(mp, obj_table, n);
>> +}
>> +
>> +/**
>> + * @internal wrapper for mempool_ops put callback.
> similar: "put callback"
Done.
- * @internal wrapper for mempool_ops put callback.
+ * @internal wrapper for mempool_ops enqueue callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @param obj_table
>> + * Pointer to a table of void * pointers (objects).
>> + * @param n
>> + * Number of objects to put.
>> + * @return
>> + * - 0: Success; n objects supplied.
>> + * - <0: Error; code of put function.
> similar: "put function"
Done.
- * - <0: Error; code of put function.
+ * - <0: Error; code of enqueue function.
>> + */
>> +static inline int
>> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const
>> *obj_table,
>> +unsigned n)
>> +{
>> +struct rte_mempool_ops *ops;
>> +
>> +ops = rte_mempool_ops_get(mp->ops_index);
>> +return ops->enqueue(mp, obj_table, n);
>> +}
>> +
> [...]
>
>> +
>> +/* add a new ops struct in rte_mempool_ops_table, return its index. */
>> +int
>> +rte_mempool_ops_register(const struct rte_mempool_ops *h)
>> +{
>> +struct rte_mempool_ops *ops;
>> +int16_t ops_index;
>> +
>> +rte_spinlock_lock(_mempool_ops_table.sl);
>> +
>> +if (rte_mempool_ops_table.num_ops >=
>> +RTE_MEMPOOL_MAX_OPS_IDX) {
>> +rte_spinlock_unlock(_mempool_ops_table.sl);
>> +RTE_LOG(ERR, MEMPOOL,
>> +"Maximum number of mempool ops structs exceeded\n");
>> +return -ENOSPC;
>> +}
>> +
>> +if (h->alloc == NULL || h->enqueue == NULL ||
>> +h->dequeue == NULL || h->get_count == NULL) {
>> +rte_spinlock_unlock(_mempool_ops_table.sl);
>> +RTE_LOG(ERR, MEMPOOL,
>> +"Missing callback while registering mempool ops\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (strlen(h->name) >= sizeof(ops->name) - 1) {
>> +RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n",
>> +__func__, h->name);
> The unlock is missing here, isn't?
Yes, well spotted. Fixed.
+ rte_spinlock_unlock(_mempool_ops_table.sl);
>> +rte_errno = EEXIST;
>> +return -EEXIST;
>> +}
>> +
>> +ops_index = rte_mempool_ops_table.num_ops++;
>> +ops = _mempool_ops_table.ops[ops_index];
>> +snprintf(ops->name, sizeof(ops->name), "%s", h->name);
>> +ops->alloc = h->alloc;
>> +ops->enqueue = h->enqueue;
>> +ops->dequeue = h->dequeue;
>> +ops->get_count = h->get_count;
>> +
>> +rte_spinlock_unlock(_mempool_ops_table.sl);
>> +
>> +return ops_index;
>> +}
>> +
>> +/* wrapper to allocate an external mempool's private (pool) data. */
>> +int
>>
[dpdk-dev] [PATCH v2 1/3] mempool: add stack (lifo) mempool handler
Hi Olivier,
On 23/5/2016 1:55 PM, Olivier Matz wrote:
> Hi David,
>
> Please find some comments below.
>
> On 05/19/2016 04:48 PM, David Hunt wrote:
>> [...]
>> +++ b/lib/librte_mempool/rte_mempool_stack.c
>> @@ -0,0 +1,145 @@
>> +/*-
>> + * BSD LICENSE
>> + *
>> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
>> + * All rights reserved.
> Should be 2016?
Yes, fixed.
>> ...
>> +
>> +static void *
>> +common_stack_alloc(struct rte_mempool *mp)
>> +{
>> +struct rte_mempool_common_stack *s;
>> +unsigned n = mp->size;
>> +int size = sizeof(*s) + (n+16)*sizeof(void *);
>> +
>> +/* Allocate our local memory structure */
>> +s = rte_zmalloc_socket("common-stack",
> "mempool-stack" ?
Done
>> +size,
>> +RTE_CACHE_LINE_SIZE,
>> +mp->socket_id);
>> +if (s == NULL) {
>> +RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
>> +return NULL;
>> +}
>> +
>> +rte_spinlock_init(>sl);
>> +
>> +s->size = n;
>> +mp->pool = s;
>> +rte_mempool_set_handler(mp, "stack");
> rte_mempool_set_handler() is a user function, it should be called here
Removed.
>> +
>> +return s;
>> +}
>> +
>> +static int common_stack_put(void *p, void * const *obj_table,
>> +unsigned n)
>> +{
>> +struct rte_mempool_common_stack *s = p;
>> +void **cache_objs;
>> +unsigned index;
>> +
>> +rte_spinlock_lock(>sl);
>> +cache_objs = >objs[s->len];
>> +
>> +/* Is there sufficient space in the stack ? */
>> +if ((s->len + n) > s->size) {
>> +rte_spinlock_unlock(>sl);
>> +return -ENOENT;
>> +}
> The usual return value for a failing put() is ENOBUFS (see in rte_ring).
Done.
>
> After reading it, I realize that it's nearly exactly the same code than
> in "app/test: test external mempool handler".
> http://patchwork.dpdk.org/dev/patchwork/patch/12896/
>
> We should drop one of them. If this stack handler is really useful for
> a performance use-case, it could go in librte_mempool. At the first
> read, the code looks like a demo example : it uses a simple spinlock for
> concurrent accesses to the common pool. Maybe the mempool cache hides
> this cost, in this case we could also consider removing the use of the
> rte_ring.
While I agree that the code is similar, the handler in the test is a
ring based handler,
where as this patch adds an array based handler.
I think that the case for leaving it in as a test for the standard
handler as part of the
previous mempool handler is valid, but maybe there is a case for
removing it if
we add the stack handler. Maybe a future patch?
> Do you have some some performance numbers? Do you know if it scales
> with the number of cores?
For the mempool_perf_autotest, I'm seeing a 30% increase in performance
for the
local cache use-case for 1 - 36 cores (results vary within those tests
between
10-45% gain, but with an average of 30% gain over all the tests.).
However, for the tests with no local cache configured, throughput of the
enqueue/dequeue
drops by about 30%, with the 36 core yelding the largest drop of 40%. So
this handler would
not be recommended in no-cache applications.
> If we can identify the conditions where this mempool handler
> overperforms the default handler, it would be valuable to have them
> in the documentation.
>
Regards,
Dave.
[dpdk-dev] [PATCH v13 1/3] mempool: support external mempool operations
On 17/6/2016 11:18 AM, Olivier Matz wrote:
> Hi David,
>
> While testing Lazaros' patch, I found an issue in this series.
> I the test application is started with --no-huge, it does not work,
> the mempool_autotest does not work. Please find the exaplanation
> below:
>
> On 06/16/2016 02:30 PM, David Hunt wrote:
>> @@ -386,9 +352,9 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
>> *vaddr,
>> int ret;
>>
>> /* create the internal ring if not already done */
>> -if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
>> -ret = rte_mempool_ring_create(mp);
>> -if (ret < 0)
>> +if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
>> +ret = rte_mempool_ops_alloc(mp);
>> +if (ret != 0)
>> return ret;
>> }
>>
> Previously, the function rte_mempool_ring_create(mp) was setting the
> MEMPOOL_F_RING_CREATED flag. Now it is not set. I think we could
> set it just after the "return ret".
>
> When started with --no-huge, the mempool memory is allocated in
> several chunks (one per page), so it tries to allocate the ring for
> each chunk.
>
> Regards,
> Olivier
OK, Will do.
ret = rte_mempool_ops_alloc(mp);
if (ret != 0)
return ret;
+ mp->flags |= MEMPOOL_F_POOL_CREATED;
Rgds,
Dave.
[dpdk-dev] [PATCH v13 1/3] mempool: support external mempool operations
On 17/6/2016 10:09 AM, Thomas Monjalon wrote: > 2016-06-17 09:42, Hunt, David: >> On 17/6/2016 9:08 AM, Olivier Matz wrote: >>> Hi David, >>> >>> On 06/17/2016 08:58 AM, Hunt, David wrote: >>>> A comment below: >>>> >>>> On 16/6/2016 1:30 PM, David Hunt wrote: >>>>> +/** >>>>> + * Set the ops of a mempool. >>>>> + * >>>>> + * This can only be done on a mempool that is not populated, i.e. >>>>> just after >>>>> + * a call to rte_mempool_create_empty(). >>>>> + * >>>>> + * @param mp >>>>> + * Pointer to the memory pool. >>>>> + * @param name >>>>> + * Name of the ops structure to use for this mempool. >>>> + * @param pool_config >>>> + * Opaque data that can be used by the ops functions. >>>>> + * @return >>>>> + * - 0: Success; the mempool is now using the requested ops functions. >>>>> + * - -EINVAL - Invalid ops struct name provided. >>>>> + * - -EEXIST - mempool already has an ops struct assigned. >>>>> + */ >>>>> +int >>>>> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, >>>>> +void *pool_config); >>>>> + >>> The changes related to the pool_config look good to me. >>> >>> If you plan to do a v14 for this API comment, I'm wondering if the >>> documentation could be slightly modified too. I think "external mempool >>> manager" was the legacy name for the feature, but maybe it could be >>> changed in "alternative mempool handlers" or "changing the mempool >>> handler". I mean the word "external" is probably not appropriate now, >>> especially if we add other handlers in the mempool lib. >>> >>> My 2 cents, >>> Olivier >> I had not planned on doing another revision. And I think the term "External >> Mempool Manager" accurately describes the functionality, so I'd really >> prefer to leave it as it is. > I think there is no manager, just a default handler which can be changed. > I agree the documentation must be fixed. OK, I have two suggestions to add into the mix. 1. mempool handler framework or simply 2. mempool handlers. (the alternative is implied). "The mempool handler feature", etc. Thoughts? David.
[dpdk-dev] [PATCH v13 1/3] mempool: support external mempool operations
On 17/6/2016 9:08 AM, Olivier Matz wrote: > Hi David, > > On 06/17/2016 08:58 AM, Hunt, David wrote: >> A comment below: >> >> On 16/6/2016 1:30 PM, David Hunt wrote: >>> +/** >>> + * Set the ops of a mempool. >>> + * >>> + * This can only be done on a mempool that is not populated, i.e. >>> just after >>> + * a call to rte_mempool_create_empty(). >>> + * >>> + * @param mp >>> + * Pointer to the memory pool. >>> + * @param name >>> + * Name of the ops structure to use for this mempool. >> + * @param pool_config >> + * Opaque data that can be used by the ops functions. >>> + * @return >>> + * - 0: Success; the mempool is now using the requested ops functions. >>> + * - -EINVAL - Invalid ops struct name provided. >>> + * - -EEXIST - mempool already has an ops struct assigned. >>> + */ >>> +int >>> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, >>> +void *pool_config); >>> + >> > The changes related to the pool_config look good to me. > > If you plan to do a v14 for this API comment, I'm wondering if the > documentation could be slightly modified too. I think "external mempool > manager" was the legacy name for the feature, but maybe it could be > changed in "alternative mempool handlers" or "changing the mempool > handler". I mean the word "external" is probably not appropriate now, > especially if we add other handlers in the mempool lib. > > My 2 cents, > Olivier I had not planned on doing another revision. And I think the term "External Mempool Manager" accurately describes the functionality, so I'd really prefer to leave it as it is. Regards, David.
[dpdk-dev] [PATCH v13 1/3] mempool: support external mempool operations
A comment below: On 16/6/2016 1:30 PM, David Hunt wrote: > +/** > + * Set the ops of a mempool. > + * > + * This can only be done on a mempool that is not populated, i.e. just after > + * a call to rte_mempool_create_empty(). > + * > + * @param mp > + * Pointer to the memory pool. > + * @param name > + * Name of the ops structure to use for this mempool. + * @param pool_config + * Opaque data that can be used by the ops functions. > + * @return > + * - 0: Success; the mempool is now using the requested ops functions. > + * - -EINVAL - Invalid ops struct name provided. > + * - -EEXIST - mempool already has an ops struct assigned. > + */ > +int > +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, > + void *pool_config); > +
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 16/6/2016 9:58 AM, Olivier MATZ wrote: >>> >>> So I don't think we should have more cache misses whether it's >>> placed at the beginning or at the end. Maybe I'm missing something... >>> >>> I still believe it's better to group the 2 fields as they are >>> tightly linked together. It could be at the end if you see better >>> performance. >>> >> >> OK, I'll leave at the end because of the performance hit. > > Sorry, my message was not clear. > I mean, having both at the end. Do you see a performance > impact in that case? > I ran multiple more tests, and average drop I'm seeing on an older server reduced to 1% average (local cached use-case), with 0% change on a newer Haswell server, so I think at this stage we're safe to put it up alongside pool_data. There was 0% reduction when I moved both to the bottom of the struct. So on the Haswell, it seems to have minimal impact regardless of where they go. I'll post the patch up soon. Regards, Dave.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 16/6/2016 9:47 AM, Olivier MATZ wrote: > > > On 06/16/2016 09:47 AM, Hunt, David wrote: >> >> >> On 15/6/2016 5:40 PM, Olivier MATZ wrote: >>> >>> >>> On 06/15/2016 06:34 PM, Hunt, David wrote: >>>> >>>> >>>> On 15/6/2016 1:03 PM, Olivier MATZ wrote: >>>>> [...] >>>>> >>>>> The opaque pointer would be saved in mempool structure, and used >>>>> when the mempool is populated (calling mempool_ops_alloc). >>>>> The type of the structure pointed by the opaque has to be defined >>>>> (and documented) into each mempool_ops manager. >>>>> >>>>> >>>>> Olivier >>>> >>>> >>>> OK, just to be sure before I post another patchset. >>>> >>>> For the rte_mempool_struct: >>>> struct rte_mempool_memhdr_list mem_list; /**< List of memory >>>> chunks */ >>>> + void *ops_args; /**< optional args for ops >>>> alloc. */ >>>> >>>> (at the end of the struct, as it's just on the control path, not to >>>> affect fast path) >>> >>> Hmm, I would put it just after pool_data. >>> >> >> When I move it to just after pool data, the performance of the >> mempool_perf_autotest drops by 2% on my machine for the local cache >> tests. >> I think I should leave it where I suggested. > > I don't really see what you call control path and data path here. > For me, all the fields in mempool structure are not modified once > the mempool is initialized. > > http://dpdk.org/browse/dpdk/tree/lib/librte_mempool/rte_mempool.h?id=ce94a51ff05c0a4b63177f8a314feb5d19992036#n201 > > > > So I don't think we should have more cache misses whether it's > placed at the beginning or at the end. Maybe I'm missing something... > > I still believe it's better to group the 2 fields as they are > tightly linked together. It could be at the end if you see better > performance. > OK, I'll leave at the end because of the performance hit. Regards, David.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 5:40 PM, Olivier MATZ wrote: > > > On 06/15/2016 06:34 PM, Hunt, David wrote: >> >> >> On 15/6/2016 1:03 PM, Olivier MATZ wrote: >>> [...] >>> >>> The opaque pointer would be saved in mempool structure, and used >>> when the mempool is populated (calling mempool_ops_alloc). >>> The type of the structure pointed by the opaque has to be defined >>> (and documented) into each mempool_ops manager. >>> >>> >>> Olivier >> >> >> OK, just to be sure before I post another patchset. >> >> For the rte_mempool_struct: >> struct rte_mempool_memhdr_list mem_list; /**< List of memory >> chunks */ >> + void *ops_args; /**< optional args for ops >> alloc. */ >> >> (at the end of the struct, as it's just on the control path, not to >> affect fast path) > > Hmm, I would put it just after pool_data. > When I move it to just after pool data, the performance of the mempool_perf_autotest drops by 2% on my machine for the local cache tests. I think I should leave it where I suggested. Regards, David.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
Hi Shreyansh, On 16/6/2016 5:35 AM, Shreyansh Jain wrote: > Though I am late to the discussion... > >> -Original Message- >> From: Olivier MATZ [mailto:olivier.matz at 6wind.com] >> Sent: Wednesday, June 15, 2016 10:10 PM >> To: Hunt, David ; Jan Viktorin >> >> Cc: dev at dpdk.org; jerin.jacob at caviumnetworks.com; Shreyansh Jain >> >> Subject: Re: [PATCH v12 0/3] mempool: add external mempool manager >> >> >> >> On 06/15/2016 06:34 PM, Hunt, David wrote: >>> >>> On 15/6/2016 1:03 PM, Olivier MATZ wrote: >>>> [...] >>>> >>>> The opaque pointer would be saved in mempool structure, and used >>>> when the mempool is populated (calling mempool_ops_alloc). >>>> The type of the structure pointed by the opaque has to be defined >>>> (and documented) into each mempool_ops manager. >>>> >>>> >>>> Olivier >>> >>> OK, just to be sure before I post another patchset. >>> >>> For the rte_mempool_struct: >>> struct rte_mempool_memhdr_list mem_list; /**< List of memory >>> chunks */ >>> + void *ops_args; /**< optional args for ops >>> alloc. */ >>> >>> (at the end of the struct, as it's just on the control path, not to >>> affect fast path) >> Hmm, I would put it just after pool_data. > +1 > And, would 'pool_config' (picked from a previous email from David) a better > name? > > From a user perspective, the application is passing a configuration item to > the pool to work one. Only the application and mempool allocator understand > it (opaque). > As for 'ops_arg', it would be to control 'assignment-of-operations' to the > framework. > > Maybe just my point of view. I agree. I was originally happy with pool_config, sits well with pool_data. And it's data for configuring the pool during allocation. I'll go with that, then. >>> Then change function params: >>>int >>> -rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name); >>> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, >>> + void *ops_args); >>> >>> And (almost) finally in the rte_mempool_set_ops_byname function: >>> mp->ops_index = i; >>> + mp->ops_args = ops_args; >>> return 0; >>> >>> Then (actually) finally, add a null to all the calls to >>> rte_mempool_set_ops_byname. >>> >>> OK? :) >>> >> Else, looks good to me! Thanks David. > Me too. Though I would like to clarify something for my understanding: > > Mempool->pool_data => Used by allocator to store private data > Mempool->pool_config => (or ops_arg) used by allocator to access user/app > provided value. > > Is that correct? Yes, that's correct. Regard, David.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 1:03 PM, Olivier MATZ wrote: > [...] > > The opaque pointer would be saved in mempool structure, and used > when the mempool is populated (calling mempool_ops_alloc). > The type of the structure pointed by the opaque has to be defined > (and documented) into each mempool_ops manager. > > > Olivier OK, just to be sure before I post another patchset. For the rte_mempool_struct: struct rte_mempool_memhdr_list mem_list; /**< List of memory chunks */ + void *ops_args; /**< optional args for ops alloc. */ (at the end of the struct, as it's just on the control path, not to affect fast path) Then change function params: int -rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name); +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, + void *ops_args); And (almost) finally in the rte_mempool_set_ops_byname function: mp->ops_index = i; + mp->ops_args = ops_args; return 0; Then (actually) finally, add a null to all the calls to rte_mempool_set_ops_byname. OK? :) Regards, Dave.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 3:47 PM, Jan Viktorin wrote:
> On Wed, 15 Jun 2016 16:10:13 +0200
> Olivier MATZ wrote:
>
>> On 06/15/2016 04:02 PM, Hunt, David wrote:
>>>
>>> On 15/6/2016 2:50 PM, Olivier MATZ wrote:
>>>> Hi David,
>>>>
>>>> On 06/15/2016 02:38 PM, Hunt, David wrote:
>>>>>
>>>>> On 15/6/2016 1:03 PM, Olivier MATZ wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 06/15/2016 01:47 PM, Hunt, David wrote:
>>>>>>>
>>>>>>> On 15/6/2016 11:13 AM, Jan Viktorin wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I've got one last question. Initially, I was interested in creating
>>>>>>>> my own external memory provider based on a Linux Kernel driver.
>>>>>>>> So, I've got an opened file descriptor that points to a device which
>>>>>>>> can mmap a memory regions for me.
>>>>>>>>
>>>>>>>> ...
>>>>>>>> int fd = open("/dev/uio0" ...);
>>>>>>>> ...
>>>>>>>> rte_mempool *pool = rte_mempool_create_empty(...);
>>>>>>>> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>>>>>>>>
>>>>>>>> I am not sure how to pass the file descriptor pointer. I thought it
>>>>>>>> would
>>>>>>>> be possible by the rte_mempool_alloc but it's not... Is it possible
>>>>>>>> to solve this case?
>>>>>>>>
>>>>>>>> The allocator is device-specific.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Jan
>>>>>>> This particular use case is not covered.
>>>>>>>
>>>>>>> We did discuss this before, and an opaque pointer was proposed, but
>>>>>>> did
>>>>>>> not make it in.
>>>>>>> http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
>>>>>>> (and following emails in that thread)
>>>>>>>
>>>>>>> So, the options for this use case are as follows:
>>>>>>> 1. Use the pool_data to pass data in to the alloc, then set the
>>>>>>> pool_data pointer before coming back from alloc. (It's a bit of a
>>>>>>> hack,
>>>>>>> but means no code change).
>>>>>>> 2. Add an extra parameter to the alloc function. The simplest way I
>>>>>>> can
>>>>>>> think of doing this is to
>>>>>>> take the *opaque passed into rte_mempool_populate_phys, and pass it on
>>>>>>> into the alloc function.
>>>>>>> This will have minimal impact on the public API,s as there is
>>>>>>> already an
>>>>>>> opaque there in the _populate_ funcs, we're just
>>>>>>> reusing it for the alloc.
>>>>>>>
>>>>>>> Do others think option 2 is OK to add this at this late stage? Even if
>>>>>>> the patch set has already been ACK'd?
>>>>>> Jan's use-case looks to be relevant.
>>>>>>
>>>>>> What about changing:
>>>>>>
>>>>>>rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>>>>>>
>>>>>> into:
>>>>>>
>>>>>> rte_mempool_set_ops(struct rte_mempool *mp, const char *name,
>>>>>> void *opaque)
> Or a third function?
>
> rte_mempool_set_ops_arg(struct rte_mempool, *mp, void *arg)
I think if we tried to add another function, there would be some
opposition to that.
I think it's reasonable to add it to set_ops_byname, as we're setting
the ops and
the ops_args in the same call. We use them later in the alloc.
>
> Or isn't it really a task for a kind of rte_mempool_populate_*?
I was leaning towards that, but a different proposal was suggested. I'm
OK with
adding the *opaque to set_ops_byname
> This is a part of mempool I am not involved in yet.
>
>>>>>> ?
>>>>>>
>>>>>> The opaque pointer would be saved in mempool structure, and used
>>>>>> when the mempool is populated (calling mempool_ops_alloc).
>>>>>> The type of the structure pointed by the opaque has to be defined
>>>&g
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 2:50 PM, Olivier MATZ wrote:
> Hi David,
>
> On 06/15/2016 02:38 PM, Hunt, David wrote:
>>
>>
>> On 15/6/2016 1:03 PM, Olivier MATZ wrote:
>>> Hi,
>>>
>>> On 06/15/2016 01:47 PM, Hunt, David wrote:
>>>>
>>>>
>>>> On 15/6/2016 11:13 AM, Jan Viktorin wrote:
>>>>> Hi,
>>>>>
>>>>> I've got one last question. Initially, I was interested in creating
>>>>> my own external memory provider based on a Linux Kernel driver.
>>>>> So, I've got an opened file descriptor that points to a device which
>>>>> can mmap a memory regions for me.
>>>>>
>>>>> ...
>>>>> int fd = open("/dev/uio0" ...);
>>>>> ...
>>>>> rte_mempool *pool = rte_mempool_create_empty(...);
>>>>> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>>>>>
>>>>> I am not sure how to pass the file descriptor pointer. I thought it
>>>>> would
>>>>> be possible by the rte_mempool_alloc but it's not... Is it possible
>>>>> to solve this case?
>>>>>
>>>>> The allocator is device-specific.
>>>>>
>>>>> Regards
>>>>> Jan
>>>>
>>>> This particular use case is not covered.
>>>>
>>>> We did discuss this before, and an opaque pointer was proposed, but
>>>> did
>>>> not make it in.
>>>> http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
>>>> (and following emails in that thread)
>>>>
>>>> So, the options for this use case are as follows:
>>>> 1. Use the pool_data to pass data in to the alloc, then set the
>>>> pool_data pointer before coming back from alloc. (It's a bit of a
>>>> hack,
>>>> but means no code change).
>>>> 2. Add an extra parameter to the alloc function. The simplest way I
>>>> can
>>>> think of doing this is to
>>>> take the *opaque passed into rte_mempool_populate_phys, and pass it on
>>>> into the alloc function.
>>>> This will have minimal impact on the public API,s as there is
>>>> already an
>>>> opaque there in the _populate_ funcs, we're just
>>>> reusing it for the alloc.
>>>>
>>>> Do others think option 2 is OK to add this at this late stage? Even if
>>>> the patch set has already been ACK'd?
>>>
>>> Jan's use-case looks to be relevant.
>>>
>>> What about changing:
>>>
>>> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>>>
>>> into:
>>>
>>> rte_mempool_set_ops(struct rte_mempool *mp, const char *name,
>>> void *opaque)
>>>
>>> ?
>>>
>>> The opaque pointer would be saved in mempool structure, and used
>>> when the mempool is populated (calling mempool_ops_alloc).
>>> The type of the structure pointed by the opaque has to be defined
>>> (and documented) into each mempool_ops manager.
>>>
>>
>> Yes, that was another option, which has the additional impact of
>> needing an
>> opaque added to the mempool struct. If we use the opaque from the
>> _populate_
>> function, we use it straight away in the alloc, no storage needed.
>>
>> Also, do you think we need to go ahead with this change, or can we add
>> it later as an
>> improvement?
>
> The opaque in populate_phys() is already used for something else
> (i.e. the argument for the free callback of the memory chunk).
> I'm afraid it could cause confusion to have it used for 2 different
> things.
>
> About the change, I think it could be good to have it in 16.11,
> because it will probably change the API, and we should avoid to
> change it each version ;)
>
> So I'd vote to have it in the patchset for consistency.
Sure, we should avoid breaking API just after we created it. :)
OK, here's a slightly different proposal.
If we add a string, to the _ops_byname, yes, that will work for Jan's case.
However, if we add a void*, that allow us the flexibility of passing
anything we
want. We can then store the void* in the mempool struct as void
*pool_config,
so that when the alloc gets called, it can access whatever is stored at
*pool_config
and do the correct initialisation/allocation. In Jan's use case, this
can simply be typecast
to a string. In future cases, it can be a struct, which could including
new flags.
I think that change is minimal enough to be low risk at this stage.
Thoughts?
Dave.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 1:03 PM, Olivier MATZ wrote:
> Hi,
>
> On 06/15/2016 01:47 PM, Hunt, David wrote:
>>
>>
>> On 15/6/2016 11:13 AM, Jan Viktorin wrote:
>>> Hi,
>>>
>>> I've got one last question. Initially, I was interested in creating
>>> my own external memory provider based on a Linux Kernel driver.
>>> So, I've got an opened file descriptor that points to a device which
>>> can mmap a memory regions for me.
>>>
>>> ...
>>> int fd = open("/dev/uio0" ...);
>>> ...
>>> rte_mempool *pool = rte_mempool_create_empty(...);
>>> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>>>
>>> I am not sure how to pass the file descriptor pointer. I thought it
>>> would
>>> be possible by the rte_mempool_alloc but it's not... Is it possible
>>> to solve this case?
>>>
>>> The allocator is device-specific.
>>>
>>> Regards
>>> Jan
>>
>> This particular use case is not covered.
>>
>> We did discuss this before, and an opaque pointer was proposed, but did
>> not make it in.
>> http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
>> (and following emails in that thread)
>>
>> So, the options for this use case are as follows:
>> 1. Use the pool_data to pass data in to the alloc, then set the
>> pool_data pointer before coming back from alloc. (It's a bit of a hack,
>> but means no code change).
>> 2. Add an extra parameter to the alloc function. The simplest way I can
>> think of doing this is to
>> take the *opaque passed into rte_mempool_populate_phys, and pass it on
>> into the alloc function.
>> This will have minimal impact on the public API,s as there is already an
>> opaque there in the _populate_ funcs, we're just
>> reusing it for the alloc.
>>
>> Do others think option 2 is OK to add this at this late stage? Even if
>> the patch set has already been ACK'd?
>
> Jan's use-case looks to be relevant.
>
> What about changing:
>
> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>
> into:
>
> rte_mempool_set_ops(struct rte_mempool *mp, const char *name,
> void *opaque)
>
> ?
>
> The opaque pointer would be saved in mempool structure, and used
> when the mempool is populated (calling mempool_ops_alloc).
> The type of the structure pointed by the opaque has to be defined
> (and documented) into each mempool_ops manager.
>
Yes, that was another option, which has the additional impact of needing an
opaque added to the mempool struct. If we use the opaque from the _populate_
function, we use it straight away in the alloc, no storage needed.
Also, do you think we need to go ahead with this change, or can we add
it later as an
improvement?
Regards,
Dave.
[dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
On 15/6/2016 11:13 AM, Jan Viktorin wrote:
> Hi,
>
> I've got one last question. Initially, I was interested in creating
> my own external memory provider based on a Linux Kernel driver.
> So, I've got an opened file descriptor that points to a device which
> can mmap a memory regions for me.
>
> ...
> int fd = open("/dev/uio0" ...);
> ...
> rte_mempool *pool = rte_mempool_create_empty(...);
> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>
> I am not sure how to pass the file descriptor pointer. I thought it would
> be possible by the rte_mempool_alloc but it's not... Is it possible
> to solve this case?
>
> The allocator is device-specific.
>
> Regards
> Jan
This particular use case is not covered.
We did discuss this before, and an opaque pointer was proposed, but did
not make it in.
http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
(and following emails in that thread)
So, the options for this use case are as follows:
1. Use the pool_data to pass data in to the alloc, then set the
pool_data pointer before coming back from alloc. (It's a bit of a hack,
but means no code change).
2. Add an extra parameter to the alloc function. The simplest way I can
think of doing this is to
take the *opaque passed into rte_mempool_populate_phys, and pass it on
into the alloc function.
This will have minimal impact on the public API,s as there is already an
opaque there in the _populate_ funcs, we're just
reusing it for the alloc.
Do others think option 2 is OK to add this at this late stage? Even if
the patch set has already been ACK'd?
Regards,
Dave.
[dpdk-dev] [PATCH v12 1/3] mempool: support external mempool operations
On 15/6/2016 11:14 AM, Jan Viktorin wrote: > On Wed, 15 Jun 2016 08:47:02 +0100 > David Hunt wrote: > [...] > >> + >> +/** Array of registered ops structs. */ >> +extern struct rte_mempool_ops_table rte_mempool_ops_table; >> + >> +/** >> + * @internal Get the mempool ops struct from its index. >> + * >> + * @param ops_index >> + * The index of the ops struct in the ops struct table. It must be a valid >> + * index: (0 <= idx < num_ops). >> + * @return >> + * The pointer to the ops struct in the table. >> + */ >> +static inline struct rte_mempool_ops * >> +rte_mempool_ops_get(int ops_index) > Shouldn't this function be called rte_mempool_get/find_ops instead? > > Jan, I think at this stage that it's probably OK as it is. :) Rgds, Dave.
[dpdk-dev] [PATCH v2 1/3] mempool: add stack (lifo) mempool handler
Hi Olivier,
On 23/5/2016 1:55 PM, Olivier Matz wrote:
> Hi David,
>
> Please find some comments below.
>
> On 05/19/2016 04:48 PM, David Hunt wrote:
>> This is a mempool handler that is useful for pipelining apps, where
>> the mempool cache doesn't really work - example, where we have one
>> core doing rx (and alloc), and another core doing Tx (and return). In
>> such a case, the mempool ring simply cycles through all the mbufs,
>> resulting in a LLC miss on every mbuf allocated when the number of
>> mbufs is large. A stack recycles buffers more effectively in this
>> case.
>>
>> v2: cleanup based on mailing list comments. Mainly removal of
>> unnecessary casts and comments.
>>
>> Signed-off-by: David Hunt
>> ---
>> lib/librte_mempool/Makefile| 1 +
>> lib/librte_mempool/rte_mempool_stack.c | 145
>> +
>> 2 files changed, 146 insertions(+)
>> create mode 100644 lib/librte_mempool/rte_mempool_stack.c
>>
>> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
>> index f19366e..5aa9ef8 100644
>> --- a/lib/librte_mempool/Makefile
>> +++ b/lib/librte_mempool/Makefile
>> @@ -44,6 +44,7 @@ LIBABIVER := 2
>> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c
>> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c
>> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_default.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_stack.c
>> # install includes
>> SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>>
>> diff --git a/lib/librte_mempool/rte_mempool_stack.c
>> b/lib/librte_mempool/rte_mempool_stack.c
>> new file mode 100644
>> index 000..6e25028
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_stack.c
>> @@ -0,0 +1,145 @@
>> +/*-
>> + * BSD LICENSE
>> + *
>> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
>> + * All rights reserved.
> Should be 2016?
Yup, will change.
>> ...
>> +
>> +static void *
>> +common_stack_alloc(struct rte_mempool *mp)
>> +{
>> +struct rte_mempool_common_stack *s;
>> +unsigned n = mp->size;
>> +int size = sizeof(*s) + (n+16)*sizeof(void *);
>> +
>> +/* Allocate our local memory structure */
>> +s = rte_zmalloc_socket("common-stack",
> "mempool-stack" ?
Yes. Also, I thing the names of the function should be changed from
common_stack_x to simply stack_x. The "common_" does not add anything.
>> +size,
>> +RTE_CACHE_LINE_SIZE,
>> +mp->socket_id);
>> +if (s == NULL) {
>> +RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
>> +return NULL;
>> +}
>> +
>> +rte_spinlock_init(>sl);
>> +
>> +s->size = n;
>> +mp->pool = s;
>> +rte_mempool_set_handler(mp, "stack");
> rte_mempool_set_handler() is a user function, it should be called here
Sure, removed.
>> +
>> +return s;
>> +}
>> +
>> +static int common_stack_put(void *p, void * const *obj_table,
>> +unsigned n)
>> +{
>> +struct rte_mempool_common_stack *s = p;
>> +void **cache_objs;
>> +unsigned index;
>> +
>> +rte_spinlock_lock(>sl);
>> +cache_objs = >objs[s->len];
>> +
>> +/* Is there sufficient space in the stack ? */
>> +if ((s->len + n) > s->size) {
>> +rte_spinlock_unlock(>sl);
>> +return -ENOENT;
>> +}
> The usual return value for a failing put() is ENOBUFS (see in rte_ring).
Done.
> After reading it, I realize that it's nearly exactly the same code than
> in "app/test: test external mempool handler".
> http://patchwork.dpdk.org/dev/patchwork/patch/12896/
>
> We should drop one of them. If this stack handler is really useful for
> a performance use-case, it could go in librte_mempool. At the first
> read, the code looks like a demo example : it uses a simple spinlock for
> concurrent accesses to the common pool. Maybe the mempool cache hides
> this cost, in this case we could also consider removing the use of the
> rte_ring.
Unlike the code in the test app, the stack handler does not use a ring.
This is for the
case where applications do a lot of core-to-core transfers of mbufs. The
test app was
simply to demonstrate a simple example of a malloc mempool handler. This
patch adds
a new lifo handler for general use.
Using the mempool_perf_autotest, I see a 30% increase in throughput when
local cache is enabled/used. However, there is up to a 50% degradation
when local cache
is NOT used, so it's not usable in all situations. However, with a 30%
gain for the cache
use-case, I think it's worth having in there as an option for people to
try if the use-case suits.
> Do you have some some performance numbers? Do you know if it scales
> with the number of cores?
30% gain when local cache is used. And these numbers scale up with the
number of cores on my test machine. It may be better for other use cases.
> If we can identify the conditions where this mempool handler
> overperforms the
[dpdk-dev] [PATCH v10 1/3] mempool: support external mempool operations
Hi Thomas,
On 14/6/2016 1:55 PM, Thomas Monjalon wrote:
> Hi David,
>
> 2016-06-14 10:46, David Hunt:
>> Until now, the objects stored in a mempool were internally stored in a
>> ring. This patch introduces the possibility to register external handlers
>> replacing the ring.
>>
>> The default behavior remains unchanged, but calling the new function
>> rte_mempool_set_handler() right after rte_mempool_create_empty() allows
>> the user to change the handler that will be used when populating
>> the mempool.
>>
>> This patch also adds a set of default ops (function callbacks) based
>> on rte_ring.
>>
>> Signed-off-by: Olivier Matz
>> Signed-off-by: David Hunt
> Glad to see we are close to have this feature integrated.
>
> I've just looked into few details before pushing.
> One of them are the comments. In mempool they were all ended by a dot.
> Please check the new comments.
Do you mean the rte_mempool struct definition, or all comments? Shall I
leave the
old comments the way they were before the change, or will I clean up?
If I clean up, I'd suggest I add a separate patch for that.
> The doc/guides/rel_notes/deprecation.rst must be updated to remove
> the deprecation notice in this patch.
Will do. As a separate patch in the set?
> Isn't there some explanations to add in
> doc/guides/prog_guide/mempool_lib.rst?
Yes, I'll adapt some of the cover letter, and add as a separate patch.
> Isn't there a better name than "default" for the default implementation?
> I don't think the filename rte_mempool_default.c is meaningful.
I could call it rte_mempool_ring.c? Since the default handler is ring based?
>> +/**
>> + * Register mempool operations
>> + *
>> + * @param h
>> + * Pointer to and ops structure to register
> The parameter name and its description are not correct.
Will fix.
>> + * @return
>> + * - >=0: Success; return the index of the ops struct in the table.
>> + * - -EINVAL - some missing callbacks while registering ops struct
>> + * - -ENOSPC - the maximum number of ops structs has been reached
>> + */
>> +int rte_mempool_ops_register(const struct rte_mempool_ops *ops);
> You can check the doc with doxygen:
> make doc-api-html
Will do.
>> --- a/lib/librte_mempool/rte_mempool_version.map
>> +++ b/lib/librte_mempool/rte_mempool_version.map
>> @@ -19,6 +19,8 @@ DPDK_2.0 {
>> DPDK_16.7 {
>> global:
>>
>> +rte_mempool_ops_table;
>> +
> Why this empty line?
No particular reason. I will remove.
>> rte_mempool_check_cookies;
>> rte_mempool_obj_iter;
>> rte_mempool_mem_iter;
>> @@ -29,6 +31,8 @@ DPDK_16.7 {
>> rte_mempool_populate_default;
>> rte_mempool_populate_anon;
>> rte_mempool_free;
>> +rte_mempool_set_ops_byname;
>> +rte_mempool_ops_register;
> Please keep it in alphabetical order.
> It seems the order was not respected before in mempool.
I will fix this also.
Regards,
Dave.
[dpdk-dev] [PATCH v5 00/10] Include resources in tests
On 13/6/2016 7:32 PM, Thomas Monjalon wrote: >> Jan Viktorin (10): >>app/test: introduce resources for tests >>mk: define objcopy-specific target and arch >>app/test: support resources externally linked >>app/test: add functions to create files from resources >>app/test: support resources archived by tar >>app/test: use linked list to store PCI drivers >>app/test: extract test_pci_setup and test_pci_cleanup >>app/test: convert current pci_test into a single test case >>eal/pci: allow to override sysfs >>app/test: do not dump PCI devices in blacklist test > Applied, thanks Hi all, Since this patch adds a dependency on libarchive-devel, should we have an entry in sys_reqs.rst? My fedora build was broken until I added that package. Regards, Dave.
[dpdk-dev] [PATCH v9 1/3] mempool: support external mempool operations
Hi Olivier,
On 13/6/2016 1:16 PM, Olivier Matz wrote:
> Hi David,
>
> Some comments below.
>
> On 06/10/2016 05:16 PM, David Hunt wrote:
>> Until now, the objects stored in a mempool were internally stored in a
>> ring. This patch introduces the possibility to register external handlers
>> replacing the ring.
>>
>> The default behavior remains unchanged, but calling the new function
>> rte_mempool_set_handler() right after rte_mempool_create_empty() allows
>> the user to change the handler that will be used when populating
>> the mempool.
>>
>> This patch also adds a set of default ops (function callbacks) based
>> on rte_ring.
>>
>> Signed-off-by: Olivier Matz
>> Signed-off-by: David Hunt
>>
>> ...
>> @@ -386,10 +352,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
>> *vaddr,
>> int ret;
>>
>> /* create the internal ring if not already done */
>> -if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
>> -ret = rte_mempool_ring_create(mp);
>> -if (ret < 0)
>> -return ret;
>> +if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
>> +rte_errno = 0;
>> +ret = rte_mempool_ops_alloc(mp);
>> +if (ret != 0) {
>> +if (rte_errno == 0)
>> +return -EINVAL;
>> +return -rte_errno;
>> +}
>> }
> The rte_errno should be removed. Just return the error code from
> rte_mempool_ops_alloc() on failure.
Done.
>> +/** Structure defining mempool operations structure */
>> +struct rte_mempool_ops {
>> +char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct */
>> +rte_mempool_alloc_t alloc; /**< Allocate private data */
>> +rte_mempool_free_t free; /**< Free the external pool. */
>> +rte_mempool_put_t put; /**< Put an object. */
>> +rte_mempool_get_t get; /**< Get an object. */
>> +rte_mempool_get_count get_count; /**< Get qty of available objs. */
>> +} __rte_cache_aligned;
>> +
> Sorry, I missed that in the previous reviews, but since the get/put
> functions have been renamed in dequeue/enqueue, I think the same change
> should also be done here.
Done. I also changed the common_ring_[sc|mc]_put and
common_ring_[sc|mc]_get.
I didn't go as far as changing the rte_mempool_put or
rte_mempool_put_bulk calls,
as they are used in several drivers and apps.
>
>> +/**
>> + * Prototype for implementation specific data provisioning function.
>> + *
>> + * The function should provide the implementation specific memory for
>> + * for use by the other mempool ops functions in a given mempool ops struct.
>> + * E.g. the default ops provides an instance of the rte_ring for this
>> purpose.
>> + * it will most likely point to a different type of data structure, and
>> + * will be transparent to the application programmer.
>> + */
>> +typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp);
> A comment saying that this function should set mp->pool_data
> would be nice here, I think.
Done
>> +/* wrapper to allocate an external mempool's private (pool) data */
>> +int
>> +rte_mempool_ops_alloc(struct rte_mempool *mp)
>> +{
>> +struct rte_mempool_ops *ops;
>> +
>> +ops = rte_mempool_ops_get(mp->ops_index);
>> +if (ops->alloc == NULL)
>> +return -ENOMEM;
>> +return ops->alloc(mp);
>> +}
> Now that we check that ops->alloc != NULL in the register function,
> I wonder if we should keep this test or not. Yes, it doesn't hurt,
> but for consistency with the other functions (get/put/get_count),
> we may remove it.
>
> This would be a good thing because it would prevent any confusion
> with rte_mempool_ops_get(), which returns a pointer to the ops struct
> (and has nothing to do with ops->get()).
Done.
>
> Regards,
> Olivier
Thanks,
Dave.
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
Hi all, On 10/6/2016 8:29 AM, Olivier Matz wrote: > Hi, > > On 06/09/2016 03:09 PM, Jan Viktorin wrote: My suggestion is to have an additional flag, 'MEMPOOL_F_PKT_ALLOC', which, if specified, would: ... #define MEMPOOL_F_SC_GET0x0008 #define MEMPOOL_F_PKT_ALLOC 0x0010 ... in rte_mempool_create_empty: ... after checking the other MEMPOOL_F_* flags... if (flags & MEMPOOL_F_PKT_ALLOC) rte_mempool_set_ops_byname(mp, RTE_MBUF_DEFAULT_MEMPOOL_OPS) And removing the redundant call to rte_mempool_set_ops_byname() in rte_pktmbuf_create_pool(). Thereafter, rte_pktmbuf_pool_create can be changed to: ... mp = rte_mempool_create_empty(name, n, elt_size, cache_size, -sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); +sizeof(struct rte_pktmbuf_pool_private), socket_id, + MEMPOOL_F_PKT_ALLOC); if (mp == NULL) return NULL; >>> Yes, this would simplify somewhat the creation of a pktmbuf pool, >>> in that it replaces the rte_mempool_set_ops_byname with a flag bit. >>> However, I'm not sure we want to introduce a third method of >>> creating a mempool to the developers. If we introduced this, we >>> would then have: 1. rte_pktmbuf_pool_create() 2. >>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which >>> would use the configured custom handler) 3. >>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set >>> followed by a call to rte_mempool_set_ops_byname() (would allow >>> several different custom handlers to be used in one application >>> >>> Does anyone else have an opinion on this? Oliver, Jerin, Jan? >> I am quite careful about this topic as I don't feel to be very >> involved in all the use cases. My opinion is that the _new API_ >> should be able to cover all cases and the _old API_ should be >> backwards compatible, however, built on top of the _new API_. >> >> I.e. I think, the flags MEMPOOL_F_SP_PUT, MEMPOOL_F_SC_GET (relicts >> of the old API) should be accepted by the old API ONLY. The >> rte_mempool_create_empty should not process them. > The rte_mempool_create_empty() function already processes these flags > (SC_GET, SP_PUT) as of today. > >> Similarly for a potential MEMPOOL_F_PKT_ALLOC, I would not polute the >> rte_mempool_create_empty by this anymore. > +1 > > I think we should stop adding flags. Flags are prefered for independent > features. Here, what would be the behavior with MEMPOOL_F_PKT_ALLOC + > MEMPOOL_F_SP_PUT? > > Another reason to not add this flag is the rte_mempool library > should not be aware of mbufs. The mbuf pools rely on mempools, but > not the contrary. > > >> In overall we would get exactly 2 approaches (and not more): >> >> * using rte_mempool_create with flags calling the >> rte_mempool_create_empty and rte_mempool_set_ops_byname internally >> (so this layer can be marked as deprecated and removed in the >> future) > Agree. This was one of the objective of my mempool rework patchset: > provide a more flexible API, and avoid functions with 10 to 15 > arguments. > >> * using rte_mempool_create_empty + rte_mempool_set_ops_byname - >> allowing any customizations but with the necessity to change the >> applications (new preferred API) > Yes. > And if required, maybe a third API is possible in case of mbuf pools. > Indeed, the applications are encouraged to use rte_pktmbuf_pool_create() > to create a pool of mbuf instead of mempool API. If an application > wants to select specific ops for it, we could add: > >rte_pktmbuf_pool_create_(..., name) > > instead of using the mempool API. > I think this is what Shreyansh suggests when he says: > >It sounds fine if calls to rte_mempool_* can select an external >handler *optionally* - but, if we pass it as command line, it would >be binding (at least, semantically) for rte_pktmbuf_* calls as well. >Isn't it? > > >> So, the old applications can stay as they are (OK, with a possible >> new flag MEMPOOL_F_PKT_ALLOC) and the new one can do the same but you >> have to set the ops explicitly. >> >> The more different ways of using those APIs we have, the greater hell >> we have to maintain. > I'm really not in favor of a MEMPOOL_F_PKT_ALLOC flag in mempool api. I would tend to agree, even though yesterday I proposed making that change. However, thinking about it some more, I'm not totally happy with the MEMPOOL_F_PKT_ALLOC addition. It adds yet another method of creating a mempool, and I think that may introduce some confusion with some developers. I also like the suggestion of rte_pktmbuf_pool_create_(..., name) suggested above, I was thinking the same myself last night, and I would prefer that rather than adding the MEMPOOL_F_PKT_ALLOC flag. Developers can add that function into their apps as a wrapper to rte_mempool_create_empty->rte_mempool_set_ops_byname should the need to have more than one pktmbuf allocator. Otherwise they can use the one that makes use of the
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
On 9/6/2016 1:30 PM, Jerin Jacob wrote: > On Thu, Jun 09, 2016 at 11:49:44AM +, Shreyansh Jain wrote: >> Hi Jerin, > Hi Shreyansh, > Yes, this would simplify somewhat the creation of a pktmbuf pool, in that >>> it replaces the rte_mempool_set_ops_byname with a flag bit. However, I'm not sure we want to introduce a third method of creating a mempool to the developers. If we introduced this, we would then have: 1. rte_pktmbuf_pool_create() 2. rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which would use the configured custom handler) 3. rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set followed by a call to rte_mempool_set_ops_byname() (would allow several different custom handlers to be used in one application Does anyone else have an opinion on this? Oliver, Jerin, Jan? >>> As I mentioned earlier, My take is not to create the separate API's for >>> external mempool handlers.In my view, It's same, just that sepreate >>> mempool handler through function pointers. >>> >>> To keep the backward compatibility, I think we can extend the flags >>> in rte_mempool_create and have a single API external/internal pool >>> creation(makes easy for existing applications too, add a just mempool >>> flag command line argument to existing applications to choose the >>> mempool handler) >> May be I am interpreting it wrong, but, are you suggesting a single mempool >> handler for all buffer/packet needs of an application (passed as command >> line argument)? >> That would be inefficient especially for cases where pool is backed by a >> hardware. The application wouldn't want its generic buffers to consume >> hardware resource which would be better used for packets. > It may vary from platform to platform or particular use case. For instance, > the HW external pool manager for generic buffers may scale better than SW > multi > producers/multi-consumer implementation when the number of cores > N > (as no locking involved in enqueue/dequeue(again it is depended on > specific HW implementation)) > > I thought their no harm in selecting the external pool handlers > in root level itself(rte_mempool_create) as by default it is > SW MP/MC and it just an option to override if the application wants it. > > Jerin > So, how about we go with the following, based on Shreyansh's suggestion: 1. Add in #define MEMPOOL_F_EMM_ALLOC 0x0010 (EMM for External Mempool Manager) 2. Check this bit in rte_mempool_create() just before the other bits are checked (by the way, the flags check has been moved to rte_mempool_create(), as per an earlier patchset, but was inadvertantly reverted) /* * First check to see if we use the config'd mempool hander. * Then examine the combinations of SP/SC/MP/MC flags to * set the correct index into the table of ops structs. */ if (flags & MEMPOOL_F_EMM_ALLOC) rte_mempool_set_ops_byname(mp, RTE_MBUF_DEFAULT_MEMPOOL_OPS) else (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) rte_mempool_set_ops_byname(mp, "ring_sp_sc"); else if (flags & MEMPOOL_F_SP_PUT) rte_mempool_set_ops_byname(mp, "ring_sp_mc"); else if (flags & MEMPOOL_F_SC_GET) rte_mempool_set_ops_byname(mp, "ring_mp_sc"); else rte_mempool_set_ops_byname(mp, "ring_mp_mc"); 3. Modify rte_pktmbuf_pool_create to pass the bit to rte_mempool_create -sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); +sizeof(struct rte_pktmbuf_pool_private), socket_id, +MEMPOOL_F_PKT_ALLOC); This will allow legacy apps to use one external handler ( as defined RTE_MBUF_DEFAULT_MEMPOOL_OPS) by adding the MEMPOOL_F_EMM_ALLOC bit to their flags in the call to rte_mempool_create(). Of course, if an app wants to use more than one external handler, they can call create_empty and set_ops_byname() for each mempool they create. Regards, Dave.
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
On 9/6/2016 12:41 PM, Shreyansh Jain wrote:
> Hi David,
>
>> -Original Message-----
>> From: Hunt, David [mailto:david.hunt at intel.com]
>> Sent: Thursday, June 09, 2016 3:10 PM
>> To: Shreyansh Jain ; dev at dpdk.org
>> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com;
>> jerin.jacob at caviumnetworks.com
>> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool
>> operations
>>
>> Hi Shreyansh,
>>
>> On 8/6/2016 2:48 PM, Shreyansh Jain wrote:
>>> Hi David,
>>>
>>> Thanks for explanation. I have some comments inline...
>>>
>>>> -Original Message-
>>>> From: Hunt, David [mailto:david.hunt at intel.com]
>>>> Sent: Tuesday, June 07, 2016 2:56 PM
>>>> To: Shreyansh Jain ; dev at dpdk.org
>>>> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com;
>>>> jerin.jacob at caviumnetworks.com
>>>> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool
>>>> operations
>>>>
>>>> Hi Shreyansh,
>>>>
>>>> On 6/6/2016 3:38 PM, Shreyansh Jain wrote:
>>>>> Hi,
>>>>>
>>>>> (Apologies for overly-eager email sent on this thread earlier. Will be
>> more
>>>> careful in future).
>>>>> This is more of a question/clarification than a comment. (And I have
>> taken
>>>> only some snippets from original mail to keep it cleaner)
>>>>>
>>>>>> +MEMPOOL_REGISTER_OPS(ops_mp_mc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_sp_sc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_mp_sc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_sp_mc);
>>>>>
>>>>>
>>>>>From the above what I understand is that multiple packet pool handlers
>> can
>>>> be created.
>>>>> I have a use-case where application has multiple pools but only the
>> packet
>>>> pool is hardware backed. Using the hardware for general buffer
>> requirements
>>>> would prove costly.
>>>>>From what I understand from the patch, selection of the pool is based
>> on
>>>> the flags below.
>>>>
>>>> The flags are only used to select one of the default handlers for
>>>> backward compatibility through
>>>> the rte_mempool_create call. If you wish to use a mempool handler that
>>>> is not one of the
>>>> defaults, (i.e. a new hardware handler), you would use the
>>>> rte_create_mempool_empty
>>>> followed by the rte_mempool_set_ops_byname call.
>>>> So, for the external handlers, you create and empty mempool, then set
>>>> the operations (ops)
>>>> for that particular mempool.
>>> I am concerned about the existing applications (for example, l3fwd).
>>> Explicit calls to 'rte_create_mempool_empty->rte_mempool_set_ops_byname'
>> model would require modifications to these applications.
>>> Ideally, without any modifications, these applications should be able to
>>> use packet pools (backed by hardware) and buffer pools (backed by
>>> ring/others) - transparently.
>>> If I go by your suggestions, what I understand is, doing the above without
>>> modification to applications would be equivalent to:
>>> struct rte_mempool_ops custom_hw_allocator = {...}
>>>
>>> thereafter, in config/common_base:
>>>
>>> CONFIG_RTE_DEFAULT_MEMPOOL_OPS="custom_hw_allocator"
>>>
>>> calls to rte_pktmbuf_pool_create would use the new allocator.
>> Yes, correct. But only for calls to rte_pktmbuf_pool_create(). Calls to
>> rte_mempool_create will continue to use the default handlers (ring based).
> Agree with you.
> But, some applications continue to use rte_mempool_create for allocating
> packet pools. Thus, even with a custom handler available (which, most
> probably, would be a hardware packet buffer handler), application would
> unintentionally end up not using it.
> Probably, such applications should be changed? (e.g. pipeline).
Yes, agreed. If those applications need to use external mempool
handlers, then they should be changed. I would see that as outside the
scope of this patchset.
>>> But, another problem arises here.
>>>
>>> There are two distinct paths for allocations of a memory pool:
>>> 1. A 'pkt' pool:
>>> rte_pktmbuf_pool_create
>>>\- rte_mempool_create_empty
>>>| \- rte_mempool_se
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
On 9/6/2016 11:31 AM, Jerin Jacob wrote:
> On Thu, Jun 09, 2016 at 10:39:46AM +0100, Hunt, David wrote:
>> Hi Shreyansh,
>>
>> On 8/6/2016 2:48 PM, Shreyansh Jain wrote:
>>> Hi David,
>>>
>>> Thanks for explanation. I have some comments inline...
>>>
>>>> -Original Message-
>>>> From: Hunt, David [mailto:david.hunt at intel.com]
>>>> Sent: Tuesday, June 07, 2016 2:56 PM
>>>> To: Shreyansh Jain ; dev at dpdk.org
>>>> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com;
>>>> jerin.jacob at caviumnetworks.com
>>>> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool
>>>> operations
>>>>
>>>> Hi Shreyansh,
>>>>
>>>> On 6/6/2016 3:38 PM, Shreyansh Jain wrote:
>>>>> Hi,
>>>>>
>>>>> (Apologies for overly-eager email sent on this thread earlier. Will be
>>>>> more
>>>> careful in future).
>>>>> This is more of a question/clarification than a comment. (And I have taken
>>>> only some snippets from original mail to keep it cleaner)
>>>>>
>>>>>> +MEMPOOL_REGISTER_OPS(ops_mp_mc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_sp_sc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_mp_sc);
>>>>>> +MEMPOOL_REGISTER_OPS(ops_sp_mc);
>>>>>
>>>>>
>>>>>From the above what I understand is that multiple packet pool handlers
>>>>> can
>>>> be created.
>>>>> I have a use-case where application has multiple pools but only the packet
>>>> pool is hardware backed. Using the hardware for general buffer requirements
>>>> would prove costly.
>>>>>From what I understand from the patch, selection of the pool is based
>>>>> on
>>>> the flags below.
>>>>
>>>> The flags are only used to select one of the default handlers for
>>>> backward compatibility through
>>>> the rte_mempool_create call. If you wish to use a mempool handler that
>>>> is not one of the
>>>> defaults, (i.e. a new hardware handler), you would use the
>>>> rte_create_mempool_empty
>>>> followed by the rte_mempool_set_ops_byname call.
>>>> So, for the external handlers, you create and empty mempool, then set
>>>> the operations (ops)
>>>> for that particular mempool.
>>> I am concerned about the existing applications (for example, l3fwd).
>>> Explicit calls to 'rte_create_mempool_empty->rte_mempool_set_ops_byname'
>>> model would require modifications to these applications.
>>> Ideally, without any modifications, these applications should be able to
>>> use packet pools (backed by hardware) and buffer pools (backed by
>>> ring/others) - transparently.
>>>
>>> If I go by your suggestions, what I understand is, doing the above without
>>> modification to applications would be equivalent to:
>>>
>>> struct rte_mempool_ops custom_hw_allocator = {...}
>>>
>>> thereafter, in config/common_base:
>>>
>>> CONFIG_RTE_DEFAULT_MEMPOOL_OPS="custom_hw_allocator"
>>>
>>> calls to rte_pktmbuf_pool_create would use the new allocator.
>> Yes, correct. But only for calls to rte_pktmbuf_pool_create(). Calls to
>> rte_mempool_create will continue to use the default handlers (ring based).
>>> But, another problem arises here.
>>>
>>> There are two distinct paths for allocations of a memory pool:
>>> 1. A 'pkt' pool:
>>> rte_pktmbuf_pool_create
>>>\- rte_mempool_create_empty
>>>| \- rte_mempool_set_ops_byname(..ring_mp_mc..)
>>>|
>>>`- rte_mempool_set_ops_byname
>>> (...RTE_MBUF_DEFAULT_MEMPOOL_OPS..)
>>> /* Override default 'ring_mp_mc' of
>>> * rte_mempool_create */
>>>
>>> 2. Through generic mempool create API
>>> rte_mempool_create
>>>\- rte_mempool_create_empty
>>> (passing pktmbuf and pool constructors)
>>> I found various instances in example applications where
>>> rte_mempool_create() is being called directly for packet pools - bypassing
>>> the more semantically correct call to rte_pktmbuf_* for packet pools.
>>>
>>> In (2) control path, RTE_MBUF_DEFAULT_MEMPOOLS_OPS wouldn't be able to
>>> repl
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
Hi Olivier,
On 8/6/2016 1:13 PM, Olivier Matz wrote:
> Hi David,
>
> Please find some comments below.
>
> On 06/03/2016 04:58 PM, David Hunt wrote:
>
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> +/**
>> + * Prototype for implementation specific data provisioning function.
>> + *
>> + * The function should provide the implementation specific memory for
>> + * for use by the other mempool ops functions in a given mempool ops struct.
>> + * E.g. the default ops provides an instance of the rte_ring for this
>> purpose.
>> + * it will mostlikely point to a different type of data structure, and
>> + * will be transparent to the application programmer.
>> + */
>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> In http://dpdk.org/ml/archives/dev/2016-June/040233.html, I suggested
> to change the prototype to return an int (-errno) and directly set
> the set mp->pool_data (void *) or mp->pool_if (uint64_t). No cast
> would be required in this latter case.
Done.
> By the way, there is a typo in the comment:
> "mostlikely" -> "most likely"
Fixed.
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_default.c
>> +static void
>> +common_ring_free(struct rte_mempool *mp)
>> +{
>> +rte_ring_free((struct rte_ring *)mp->pool_data);
>> +}
> I don't think the cast is needed here.
> (same in other functions)
Removed.
>
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_ops.c
>> +/* add a new ops struct in rte_mempool_ops_table, return its index */
>> +int
>> +rte_mempool_ops_register(const struct rte_mempool_ops *h)
>> +{
>> +struct rte_mempool_ops *ops;
>> +int16_t ops_index;
>> +
>> +rte_spinlock_lock(_mempool_ops_table.sl);
>> +
>> +if (rte_mempool_ops_table.num_ops >=
>> +RTE_MEMPOOL_MAX_OPS_IDX) {
>> +rte_spinlock_unlock(_mempool_ops_table.sl);
>> +RTE_LOG(ERR, MEMPOOL,
>> +"Maximum number of mempool ops structs exceeded\n");
>> +return -ENOSPC;
>> +}
>> +
>> +if (h->put == NULL || h->get == NULL || h->get_count == NULL) {
>> +rte_spinlock_unlock(_mempool_ops_table.sl);
>> +RTE_LOG(ERR, MEMPOOL,
>> +"Missing callback while registering mempool ops\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (strlen(h->name) >= sizeof(ops->name) - 1) {
>> +RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n",
>> +__func__, h->name);
>> +rte_errno = EEXIST;
>> +return NULL;
>> +}
> This has already been noticed by Shreyansh, but in case of:
>
> rte_mempool_ops.c:75:10: error: return makes integer from pointer
> without a cast [-Werror=int-conversion]
> return NULL;
>^
Changed to return -EEXIST
>
>> +/* sets mempool ops previously registered by rte_mempool_ops_register */
>> +int
>> +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>
> When I compile with shared libraries enabled, I get the following error:
>
> librte_reorder.so: undefined reference to `rte_mempool_ops_table'
> librte_mbuf.so: undefined reference to `rte_mempool_set_ops_byname'
> ...
>
> The new functions and global variables must be in
> rte_mempool_version.map. This was in v5
> ( http://dpdk.org/ml/archives/dev/2016-May/039365.html ) but
> was dropped in v6.
OK, Added.
>
> Regards,
> Olivier
Thanks,
David.
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
Hi Shreyansh,
On 8/6/2016 2:48 PM, Shreyansh Jain wrote:
> Hi David,
>
> Thanks for explanation. I have some comments inline...
>
>> -Original Message-----
>> From: Hunt, David [mailto:david.hunt at intel.com]
>> Sent: Tuesday, June 07, 2016 2:56 PM
>> To: Shreyansh Jain ; dev at dpdk.org
>> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com;
>> jerin.jacob at caviumnetworks.com
>> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool
>> operations
>>
>> Hi Shreyansh,
>>
>> On 6/6/2016 3:38 PM, Shreyansh Jain wrote:
>>> Hi,
>>>
>>> (Apologies for overly-eager email sent on this thread earlier. Will be more
>> careful in future).
>>> This is more of a question/clarification than a comment. (And I have taken
>> only some snippets from original mail to keep it cleaner)
>>>
>>>> +MEMPOOL_REGISTER_OPS(ops_mp_mc);
>>>> +MEMPOOL_REGISTER_OPS(ops_sp_sc);
>>>> +MEMPOOL_REGISTER_OPS(ops_mp_sc);
>>>> +MEMPOOL_REGISTER_OPS(ops_sp_mc);
>>>
>>>
>>> From the above what I understand is that multiple packet pool handlers can
>> be created.
>>> I have a use-case where application has multiple pools but only the packet
>> pool is hardware backed. Using the hardware for general buffer requirements
>> would prove costly.
>>> From what I understand from the patch, selection of the pool is based on
>> the flags below.
>>
>> The flags are only used to select one of the default handlers for
>> backward compatibility through
>> the rte_mempool_create call. If you wish to use a mempool handler that
>> is not one of the
>> defaults, (i.e. a new hardware handler), you would use the
>> rte_create_mempool_empty
>> followed by the rte_mempool_set_ops_byname call.
>> So, for the external handlers, you create and empty mempool, then set
>> the operations (ops)
>> for that particular mempool.
> I am concerned about the existing applications (for example, l3fwd).
> Explicit calls to 'rte_create_mempool_empty->rte_mempool_set_ops_byname'
> model would require modifications to these applications.
> Ideally, without any modifications, these applications should be able to use
> packet pools (backed by hardware) and buffer pools (backed by ring/others) -
> transparently.
>
> If I go by your suggestions, what I understand is, doing the above without
> modification to applications would be equivalent to:
>
>struct rte_mempool_ops custom_hw_allocator = {...}
>
> thereafter, in config/common_base:
>
>CONFIG_RTE_DEFAULT_MEMPOOL_OPS="custom_hw_allocator"
>
> calls to rte_pktmbuf_pool_create would use the new allocator.
Yes, correct. But only for calls to rte_pktmbuf_pool_create(). Calls to
rte_mempool_create will continue to use the default handlers (ring based).
> But, another problem arises here.
>
> There are two distinct paths for allocations of a memory pool:
> 1. A 'pkt' pool:
> rte_pktmbuf_pool_create
> \- rte_mempool_create_empty
> | \- rte_mempool_set_ops_byname(..ring_mp_mc..)
> |
> `- rte_mempool_set_ops_byname
> (...RTE_MBUF_DEFAULT_MEMPOOL_OPS..)
> /* Override default 'ring_mp_mc' of
> * rte_mempool_create */
>
> 2. Through generic mempool create API
> rte_mempool_create
> \- rte_mempool_create_empty
> (passing pktmbuf and pool constructors)
>
> I found various instances in example applications where rte_mempool_create()
> is being called directly for packet pools - bypassing the more semantically
> correct call to rte_pktmbuf_* for packet pools.
>
> In (2) control path, RTE_MBUF_DEFAULT_MEMPOOLS_OPS wouldn't be able to
> replace custom handler operations for packet buffer allocations.
>
> From a performance point-of-view, Applications should be able to select
> between packet pools and non-packet pools.
This is intended for backward compatibility, and API consistency. Any
applications that use
rte_mempool_create directly will continue to use the default mempool
handlers. If the need
to use a custeom hander, they will need to be modified to call the newer
API,
rte_mempool_create_empty and rte_mempool_set_ops_byname.
>>>
>>>> + /*
>>>> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>>>> + * set the correct index into the table of ops structs.
>>>> + */
>>>> + if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>>>> + rte_mempool_set_ops_byname(mp, "ring_sp_sc");
>>>> + else
[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations
Hi Shreyansh, On 6/6/2016 3:38 PM, Shreyansh Jain wrote: > Hi, > > (Apologies for overly-eager email sent on this thread earlier. Will be more > careful in future). > > This is more of a question/clarification than a comment. (And I have taken > only some snippets from original mail to keep it cleaner) > > >> +MEMPOOL_REGISTER_OPS(ops_mp_mc); >> +MEMPOOL_REGISTER_OPS(ops_sp_sc); >> +MEMPOOL_REGISTER_OPS(ops_mp_sc); >> +MEMPOOL_REGISTER_OPS(ops_sp_mc); > > > From the above what I understand is that multiple packet pool handlers can > be created. > > I have a use-case where application has multiple pools but only the packet > pool is hardware backed. Using the hardware for general buffer requirements > would prove costly. > From what I understand from the patch, selection of the pool is based on the > flags below. The flags are only used to select one of the default handlers for backward compatibility through the rte_mempool_create call. If you wish to use a mempool handler that is not one of the defaults, (i.e. a new hardware handler), you would use the rte_create_mempool_empty followed by the rte_mempool_set_ops_byname call. So, for the external handlers, you create and empty mempool, then set the operations (ops) for that particular mempool. > >> +/* >> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to >> + * set the correct index into the table of ops structs. >> + */ >> +if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) >> +rte_mempool_set_ops_byname(mp, "ring_sp_sc"); >> +else if (flags & MEMPOOL_F_SP_PUT) >> +rte_mempool_set_ops_byname(mp, "ring_sp_mc"); >> +else if (flags & MEMPOOL_F_SC_GET) >> +rte_mempool_set_ops_byname(mp, "ring_mp_sc"); >> +else >> +rte_mempool_set_ops_byname(mp, "ring_mp_mc"); >> + > Is there any way I can achieve the above use case of multiple pools which can > be selected by an application - something like a run-time toggle/flag? > > - > Shreyansh Yes, you can create multiple pools, some using the default handlers, and some using external handlers. There is an example of this in the autotests (app/test/test_mempool.c). This test creates multiple mempools, of which one is a custom malloc based mempool handler. The test puts and gets mbufs to/from each mempool all in the same application. Regards, Dave.
[dpdk-dev] [PATCH v7 2/5] mempool: remove rte_ring from rte_mempool struct
On 6/3/2016 1:28 PM, Olivier MATZ wrote:
>
> On 06/02/2016 03:27 PM, David Hunt wrote:
>> Now that we're moving to an external mempoool handler, which
>> uses a void *pool_data as a pointer to the pool data, remove the
>> unneeded ring pointer from the mempool struct.
>>
>> Signed-off-by: David Hunt
>> ---
>> app/test/test_mempool_perf.c | 1 -
>> lib/librte_mempool/rte_mempool.h | 1 -
>> 2 files changed, 2 deletions(-)
>>
>> diff --git a/app/test/test_mempool_perf.c b/app/test/test_mempool_perf.c
>> index cdc02a0..091c1df 100644
>> --- a/app/test/test_mempool_perf.c
>> +++ b/app/test/test_mempool_perf.c
>> @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
>> n_get_bulk);
>> if (unlikely(ret < 0)) {
>> rte_mempool_dump(stdout, mp);
>> -rte_ring_dump(stdout, mp->ring);
>> /* in this case, objects are lost... */
>> return -1;
>> }
>> diff --git a/lib/librte_mempool/rte_mempool.h
>> b/lib/librte_mempool/rte_mempool.h
>> index a6b28b0..c33eeb8 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -204,7 +204,6 @@ struct rte_mempool_memhdr {
>>*/
>> struct rte_mempool {
>> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>> -struct rte_ring *ring; /**< Ring to store objects. */
>> union {
>> void *pool_data; /**< Ring or pool to store objects */
>> uint64_t pool_id;/**< External mempool identifier */
>>
> Sorry if I missed it in previous discussions, but I don't really
> see the point of having this in a separate commit, as the goal
> of the previous commit is to replace the ring by configurable ops.
>
> Moreover, after applying only the previous commit, the
> call to rte_ring_dump(stdout, mp->ring) would probably crash
> sine ring is NULL.
>
> I think this comment also applies to the next commit. Splitting
> between functionalities is good, but in this case I think the 3
> commits are linked together, and it should not break compilation
> or tests to facilitate the git bisect.
>
>
> Regards,
> Olivier
Yes. Originally there was a lot of discussion to split out the bigger
patch, which I
did, and it was easier to review, but I think that now we're (very)
close to final
revision, I can merge those three back into one.
Thanks,
Dave.
[dpdk-dev] [PATCH v7 5/5] mbuf: allow apps to change default mempool ops
On 6/3/2016 3:10 PM, Olivier Matz wrote: > > On 06/03/2016 04:06 PM, Hunt, David wrote: >> >> On 6/3/2016 1:28 PM, Olivier MATZ wrote: >>>> [PATCH v7 5/5] mbuf: allow apps to change default mempool ops >>> Should the title be fixed? >>> I don't feel this allows application to change the default ops. >> Allow _user_ to change default mempool ops, I think. > make default mempool ops configurable at build > ? Yup :)
[dpdk-dev] [PATCH v7 5/5] mbuf: allow apps to change default mempool ops
On 6/3/2016 1:28 PM, Olivier MATZ wrote: >> [PATCH v7 5/5] mbuf: allow apps to change default mempool ops > Should the title be fixed? > I don't feel this allows application to change the default ops. Allow _user_ to change default mempool ops, I think.
[dpdk-dev] [PATCH v7 1/5] mempool: support external mempool operations
On 6/3/2016 12:07 PM, Olivier MATZ wrote:
>
> On 06/03/2016 12:28 PM, Hunt, David wrote:
>> On 6/3/2016 7:38 AM, Jerin Jacob wrote:
>>> On Thu, Jun 02, 2016 at 02:27:19PM +0100, David Hunt wrote:
>>>> +/**
>>>> + * @internal wrapper for external mempool manager put callback.
>>>> + *
>>>> + * @param mp
>>>> + * Pointer to the memory pool.
>>>> + * @param obj_table
>>>> + * Pointer to a table of void * pointers (objects).
>>>> + * @param n
>>>> + * Number of objects to put.
>>>> + * @return
>>>> + * - 0: Success; n objects supplied.
>>>> + * - <0: Error; code of put function.
>>>> + */
>>>> +static inline int
>>>> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const
>>>> *obj_table,
>>>> +unsigned n)
>>>> +{
>>>> +struct rte_mempool_ops *ops;
>>>> +
>>>> +ops = rte_mempool_ops_get(mp->ops_index);
>>>> +return ops->put(mp->pool_data, obj_table, n);
>>> Pass by value of "pool_data", On 32 bit systems, casting back to
>>> pool_id will
>>> be an issue as void* on 32 bit is 4B. IMO, May be can use uint64_t to
>>> pass by value and typecast to void* to fix it.
>> OK. I see the problem. I'll see 4 callbacks that need to change, free,
>> get, put and get_count.
>> So the callbacks will be:
>> typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>> typedef void (*rte_mempool_free_t)(uint64_t p);
>> typedef int (*rte_mempool_put_t)(uint64_t p, void * const *obj_table,
>> unsigned int n);
>> typedef int (*rte_mempool_get_t)(uint64_t p, void **obj_table, unsigned
>> int n);
>> typedef unsigned (*rte_mempool_get_count)(uint64_t p);
> I don't quite like the uint64_t argument (I exepect that most handlers
> will use a pointer, and they will have to do a cast). What about giving
> a (struct rte_mempool *) instead? The handler function would then
> select between void * or uint64_t without a cast.
> In that case, maybe the prototype of alloc should be:
>
>typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp);
>
> It would directly set mp->pool_data and return 0 or -errno.
I would tend to agree. The uint64 didn't sit well with me :)
I would prefer the rte_mempool*
> By the way, I found a strange thing:
>
>> typedef void (*rte_mempool_free_t)(void *p);
Yes, I spotted that earler, will be fixed in next version
> [...]
>
>> void
>> rte_mempool_ops_free(struct rte_mempool *mp)
>> {
>> struct rte_mempool_ops *ops;
>>
>> ops = rte_mempool_ops_get(mp->ops_index);
>> if (ops->free == NULL)
>> return;
>> return ops->free(mp);
>> }
>>
> Seems that the free cb expects mp->pool_data, but mp is passed.
Working in it.
>
>
> Another alternative to the "uint64_t or ptr" question would be to use
> a uintptr_t instead of a uint64_t. This is won't be possible if it need
> to be a 64 bits value even on 32 bits architectures. We could then keep
> the argument as pointer, and cast it to uintptr_t if needed.
I had assumed that the requirement was for 64 bits even on 32 bit OS's.
I've implemented
the double cast of the u64 to uintptr_t to struct pointer done to avoid
compiler warnings on
32-bit but I really prefer the solution of passing the rte_mempool
pointer instead. I'll change to
*mp.
Regards,
Dave.
[dpdk-dev] [PATCH v7 1/5] mempool: support external mempool operations
On 6/3/2016 7:38 AM, Jerin Jacob wrote:
> On Thu, Jun 02, 2016 at 02:27:19PM +0100, David Hunt wrote:
>
> [snip]
>
>> /* create the internal ring if not already done */
>> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> |> 1) May be RING can be replaced with some other higher abstraction name
> |> for the internal MEMPOOL_F_RING_CREATED flag
> |
> |Agreed. I'll change to MEMPOOL_F_POOL_CREATED, since we're already
> |changing the *ring
> |element of the mempool struct to *pool
>
> Looks like you have missed the above review comment?
Ah, yes, I'll address in the next patch.
I've to remove some 'const's in the alloc functions anyway
which I introduced in the last revision, which I shouldn't have. Future
handlers
(including the stack hander) will need to set the pool_data in the alloc.
>
> [snip]
>
>> static inline struct rte_mempool_ops *
>> rte_mempool_ops_get(int ops_index)
>>
>> return _mempool_ops_table.ops[ops_index];
> |> 2) Considering "get" and "put" are the fast-path callbacks for
> |> pool-manger, Is it possible to avoid the extra overhead of the
> |> following
> |> _load_ and additional cache line on each call,
> |> rte_mempool_handler_table.handler[handler_idx]
> |>
> |> I understand it is for multiprocess support but I am thing can we
> |> introduce something like ethernet API support for multiprocess and
> |> resolve "put" and "get" functions pointer on init and store in
> |> struct mempool. Some thinking like
> |>
> |> file: drivers/net/ixgbe/ixgbe_ethdev.c
> |> search for if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> |
> |I'll look at this one before posting the next version of the patch
> |(soon). :)
>
> Have you checked the above comment, if it difficult then postpone it. But
> IMO it will reduce few cycles in fast-path and reduce the cache usage in
> fast path
>
> [snip]
I looked at it, but I'd need to do some more digging into it to see how
it can be
done properly. I'm not seeing any performance drop at the moment, and it
may
be a good way to improve further down the line. Is it OK to postpone?
>> +
>> +/**
>> + * @internal wrapper for external mempool manager put callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @param obj_table
>> + * Pointer to a table of void * pointers (objects).
>> + * @param n
>> + * Number of objects to put.
>> + * @return
>> + * - 0: Success; n objects supplied.
>> + * - <0: Error; code of put function.
>> + */
>> +static inline int
>> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const
>> *obj_table,
>> +unsigned n)
>> +{
>> +struct rte_mempool_ops *ops;
>> +
>> +ops = rte_mempool_ops_get(mp->ops_index);
>> +return ops->put(mp->pool_data, obj_table, n);
> Pass by value of "pool_data", On 32 bit systems, casting back to pool_id will
> be an issue as void* on 32 bit is 4B. IMO, May be can use uint64_t to
> pass by value and typecast to void* to fix it.
OK. I see the problem. I'll see 4 callbacks that need to change, free,
get, put and get_count.
So the callbacks will be:
typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
typedef void (*rte_mempool_free_t)(uint64_t p);
typedef int (*rte_mempool_put_t)(uint64_t p, void * const *obj_table,
unsigned int n);
typedef int (*rte_mempool_get_t)(uint64_t p, void **obj_table, unsigned
int n);
typedef unsigned (*rte_mempool_get_count)(uint64_t p);
Regards,
Dave.
[dpdk-dev] [PATCH v7 0/5] mempool: add external mempool manager
Since the cover letter seems to have gone missing, sending it again: Here's the latest version of the External Mempool Manager patchset. It's re-based on top of the latest head as of 19/5/2016, including Olivier's 35-part patch series on mempool re-org [1] [1] http://dpdk.org/ml/archives/dev/2016-May/039229.html v7 changes: * Changed rte_mempool_handler_table to rte_mempool_ops_table * Changed hander_idx to ops_index in rte_mempool struct * Reworked comments in rte_mempool.h around ops functions * Changed rte_mempool_hander.c to rte_mempool_ops.c * Changed all functions containing _handler_ to _ops_ * Now there is no mention of 'handler' left * Other small changes out of review of mailing list v6 changes: * Moved the flags handling from rte_mempool_create_empty to rte_mempool_create, as it's only there for backward compatibility * Various comment additions and cleanup * Renamed rte_mempool_handler to rte_mempool_ops * Added a union for *pool and u64 pool_id in struct rte_mempool * split the original patch into a few parts for easier review. * rename functions with _ext_ to _ops_. * addressed review comments * renamed put and get functions to enqueue and dequeue * changed occurences of rte_mempool_ops to const, as they contain function pointers (security) * split out the default external mempool handler into a separate patch for easier review v5 changes: * rebasing, as it is dependent on another patch series [1] v4 changes (Olivier Matz): * remove the rte_mempool_create_ext() function. To change the handler, the user has to do the following: - mp = rte_mempool_create_empty() - rte_mempool_set_handler(mp, "my_handler") - rte_mempool_populate_default(mp) This avoids to add another function with more than 10 arguments, duplicating the doxygen comments * change the api of rte_mempool_alloc_t: only the mempool pointer is required as all information is available in it * change the api of rte_mempool_free_t: remove return value * move inline wrapper functions from the .c to the .h (else they won't be inlined). This implies to have one header file (rte_mempool.h), or it would have generate cross dependencies issues. * remove now unused MEMPOOL_F_INT_HANDLER (note: it was misused anyway due to the use of && instead of &) * fix build in debug mode (__MEMPOOL_STAT_ADD(mp, put_pool, n) remaining) * fix build with shared libraries (global handler has to be declared in the .map file) * rationalize #include order * remove unused function rte_mempool_get_handler_name() * rename some structures, fields, functions * remove the static in front of rte_tailq_elem rte_mempool_tailq (comment from Yuanhan) * test the ext mempool handler in the same file than standard mempool tests, avoiding to duplicate the code * rework the custom handler in mempool_test * rework a bit the patch selecting default mbuf pool handler * fix some doxygen comments v3 changes: * simplified the file layout, renamed to rte_mempool_handler.[hc] * moved the default handlers into rte_mempool_default.c * moved the example handler out into app/test/test_ext_mempool.c * removed is_mc/is_mp change, slight perf degredation on sp cached operation * removed stack hanler, may re-introduce at a later date * Changes out of code reviews v2 changes: * There was a lot of duplicate code between rte_mempool_xmem_create and rte_mempool_create_ext. This has now been refactored and is now hopefully cleaner. * The RTE_NEXT_ABI define is now used to allow building of the library in a format that is compatible with binaries built against previous versions of DPDK. * Changes out of code reviews. Hopefully I've got most of them included. The External Mempool Manager is an extension to the mempool API that allows users to add and use an external mempool manager, which allows external memory subsystems such as external hardware memory management systems and software based memory allocators to be used with DPDK. The existing API to the internal DPDK mempool manager will remain unchanged and will be backward compatible. However, there will be an ABI breakage, as the mempool struct is changing. These changes are all contained withing RTE_NEXT_ABI defs, and the current or next code can be changed with the CONFIG_RTE_NEXT_ABI config setting There are two aspects to external mempool manager. 1. Adding the code for your new mempool operations (ops). This is achieved by adding a new mempool ops source file into the librte_mempool library, and using the REGISTER_MEMPOOL_HANDLER macro. 2. Using the new API to call rte_mempool_create_empty and rte_mempool_set_ops to create a new mempool using the name parameter to identify which ops to use. New API calls added 1. A new rte_mempool_create_empty() function 2. rte_mempool_set_ops_byname() which sets the mempool's ops (functions)
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 6:54 PM, Jan Viktorin wrote:
>
> mp->populated_size--;
> @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
> *vaddr,
> unsigned i = 0;
> size_t off;
> struct rte_mempool_memhdr *memhdr;
> - int ret;
>
> /* create the internal ring if not already done */
> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> - ret = rte_mempool_ring_create(mp);
> - if (ret < 0)
> - return ret;
> + rte_errno = 0;
> + mp->pool = rte_mempool_ops_alloc(mp);
> + if (mp->pool == NULL) {
> + if (rte_errno == 0)
> + return -EINVAL;
> + return -rte_errno;
> Are you sure the rte_errno is a positive value?
If the user returns EINVAL, or similar, we want to return negative, as
per the rest of DPDK.
>> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr {
>> struct rte_mempool {
>> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>> struct rte_ring *ring; /**< Ring to store objects. */
>> +union {
>> +void *pool; /**< Ring or pool to store objects */
> What about calling this pdata or priv? I think, it can improve some doc
> comments.
> Describing a "pool" may refer to both the rte_mempool itself or to the
> mp->pool
> pointer. The "priv" would help to understand the code a bit.
>
> Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or
> something
> similar. It's than clear enough, what the function should do...
I'd lean towards pdata or maybe pool_data. Not sure about the function
name change though,
the function does return a pool_data pointer, which we put into
mp->pool_data.
>> +uint64_t *pool_id; /**< External mempool identifier */
> Why is pool_id a pointer? Is it a typo? I've understood it should be 64b wide
> from the discussion (Olivier, Jerin, David):
Yes, typo.
> uint32_t trailer_size; /**< Size of trailer (after elt). */
>
> unsigned private_data_size; /**< Size of private data. */
> + /**
> + * Index into rte_mempool_handler_table array of mempool handler ops
> s/rte_mempool_handler_table/rte_mempool_ops_table/
Done.
>> + * structs, which contain callback function pointers.
>> + * We're using an index here rather than pointers to the callbacks
>> + * to facilitate any secondary processes that may want to use
>> + * this mempool.
>> + */
>> +int32_t handler_idx;
> s/handler_idx/ops_idx/
>
> What about ops_index? Not a big deal...
I agree ops_index is better. Changed.
>>
>> struct rte_mempool_cache *local_cache; /**< Per-lcore local cache */
>>
>> @@ -325,6 +338,199 @@ void rte_mempool_check_cookies(const struct
>> rte_mempool *mp,
>> #define __mempool_check_cookies(mp, obj_table_const, n, free) do {}
>> while(0)
>> #endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>>
>> +#define RTE_MEMPOOL_HANDLER_NAMESIZE 32 /**< Max length of handler name. */
>> +
>> +/**
>> + * typedef for allocating the external pool.
> What about:
>
> function prototype to provide an implementation specific data
>
>> + * The handler's alloc function does whatever it needs to do to grab memory
>> + * for this handler, and sets the *pool opaque pointer in the rte_mempool
>> + * struct. In the default handler, *pool points to a ring,in other handlers,
> What about:
>
> The function should provide a memory heap representation or another private
> data
> used for allocation by the rte_mempool_ops. E.g. the default ops provides an
> instance of the rte_ring for this purpose.
>
>> + * it will mostlikely point to a different type of data structure.
>> + * It will be transparent to the application programmer.
> I'd add something like this:
>
> The function should not touch the given *mp instance.
Agreed. Reworked somewhat.
> ...because it's goal is NOT to set the mp->pool, this is done by the
> rte_mempool_populate_phys - the caller of this rte_mempool_ops_alloc.
>
> This is why I've suggested to pass the rte_mempool as const in the v5.
> Is there any reason to modify the rte_mempool contents by the implementation?
> I think, we just need to read the flags, socket_id, etc.
Yes, I agree it should be const. Changed.
>> + */
>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>> +
>> +/** Free the external pool opaque data (that pointed to by *pool) */
> What about:
>
> /** Free the opaque private data stored in the mp->pool pointer. */
I've merged the two versions of the comment:
/** Free the opaque private data pointed to by mp->pool_data pointer */
>> +typedef void (*rte_mempool_free_t)(void *p);
>> +
>> +/**
>> + * Put an object in the external pool.
>> + * The *p pointer is the opaque data for a given mempool handler (ring,
>> + * array, linked list, etc)
> The obj_table is not documented. Is it really a table? I'd called an
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 6:54 PM, Jan Viktorin wrote: > Hello David, > > the rename s/handler/ops/ has a lot of residues. Sorry for that :). I tried to > mark most of them. Otherwise, I couldn't see many more serious issues for now. Ah, I had assumed that we were just talking about the rte_mempool_handler_ops structure, not a global replace of 'handler' with 'ops'. It does make sense to change it to ops, so we don't have two words describing the same entity. I'll change to ops. Just, note the s/pool/priv/ rename suggestion. I prefer your suggestion of pdata rather than priv, how about "pool_data"? Again, thanks for the comprehensive review. Regards, Dave. [...]
[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
On 6/1/2016 5:19 PM, David Hunt wrote: > Until now, the objects stored in a mempool were internally stored in a > ring. This patch introduces the possibility to register external handlers > replacing the ring. > > The default behavior remains unchanged, but calling the new function > rte_mempool_set_handler() right after rte_mempool_create_empty() allows > the user to change the handler that will be used when populating > the mempool. > > v7 changes: >* Moved the flags handling from rte_mempool_create_empty to > rte_mempool_create, as it's only there for backward compatibility >* Various comment additions and cleanup >* Renamed rte_mempool_handler to rte_mempool_ops >* Added a union for *pool and u64 pool_id in struct rte_mempool These v7 changes should me merged with the v6 changes below as this is a v6 patch. Or removed altogether, as they are in the cover letter. > v6 changes: >* split the original patch into a few parts for easier review. >* rename functions with _ext_ to _ops_. >* addressed some review comments >* renamed put and get functions to enqueue and dequeue >* renamed rte_mempool_handler struct to rte_mempool_handler_ops >* changed occurences of rte_mempool_handler_ops to const, as they > contain function pointers (security) >* added some extra comments > > [...]
[dpdk-dev] [PATCH v5 1/3] mempool: support external handler
On 5/31/2016 10:11 PM, Jerin Jacob wrote: > On Tue, May 31, 2016 at 10:41:00PM +0200, Olivier MATZ wrote: >> Hi, >> >> On 05/31/2016 06:03 PM, Jerin Jacob wrote: >>> On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote: >>>> >>>> On 5/31/2016 9:53 AM, Jerin Jacob wrote: >>>>> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote: >>>>>> New mempool handlers will use rte_mempool_create_empty(), >>>>>> rte_mempool_set_handler(), >>>>>> then rte_mempool_populate_*(). These three functions are new to this >>>>>> release, to no problem >>>>> Having separate APIs for external pool-manager create is worrisome in >>>>> application perspective. Is it possible to have rte_mempool_[xmem]_create >>>>> for the both external and existing SW pool manager and make >>>>> rte_mempool_create_empty and rte_mempool_populate_* internal functions. >>>>> >>>>> IMO, We can do that by selecting specific rte_mempool_set_handler() >>>>> based on _flags_ encoding, something like below >>>>> >>>>> bit 0 - 16 // generic bits uses across all the pool managers >>>>> bit 16 - 23 // pool handler specific flags bits >>>>> bit 24 - 31 // to select the specific pool manager(Up to 256 different >>>>> flavors of >>>>> pool managers, For backward compatibility, make '0'(in 24-31) to select >>>>> existing SW pool manager. >>>>> >>>>> and applications can choose the handlers by selecting the flag in >>>>> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any >>>>> other >>>>> applications to choose the pool handler from command line etc in future. >>>> There might be issues with the 8-bit handler number, as we'd have to add an >>>> api call to >>>> first get the index of a given hander by name, then OR it into the flags. >>>> That would mean >>>> also extra API calls for the non-default external handlers. I do agree with >>>> the handler-specific >>>> bits though. >>> That would be an internal API(upper 8 bits to handler name). Right ? >>> Seems to be OK for me. >>> >>>> Having the _empty and _set_handler APIs seems to me to be OK for the >>>> moment. Maybe Olivier could comment? >>>> >>> But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe >>> it is better reduce the public API in spec where ever possible ? >>> >>> Maybe Olivier could comment ? >> Well, I think having 3 different functions is not a problem if the API >> is clearer. >> >> In my opinion, the following: >> rte_mempool_create_empty() >> rte_mempool_set_handler() >> rte_mempool_populate() >> >> is clearer than: >> rte_mempool_create(15 args) > But proposed scheme is not adding any new arguments to > rte_mempool_create. It just extending the existing flag. > > rte_mempool_create(15 args) is still their as API for internal pool > creation. > >> Splitting the flags into 3 groups, with one not beeing flags but a >> pool handler number looks overcomplicated from a user perspective. > I am concerned with seem less integration with existing applications, > IMO, Its not worth having separate functions for external vs internal > pool creation for application(now each every applications has to added this > logic every where for no good reason), just my 2 cents. I think that there is always going to be some extra code in the applications that want to use an external mempool. The _set_handler approach does create, set_hander, populate. The Flags method queries the handler list to get the index, sets the flags bits, then calls create. Both methods will work. But I think the _set_handler approach is more user friendly, therefore that it the method I would lean towards. >>>>> and we can remove "mbuf: get default mempool handler from configuration" >>>>> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined >>>>> then set >>>>> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create. >>>>> >>>>> What do you think? >>>> The "configuration" patch is to allow users to quickly change the mempool >>>> handler >>>> by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known >>>> handler. It could just as
[dpdk-dev] [dpdk-dev,v5,1/3] mempool: support external handler
On 5/31/2016 9:40 PM, Olivier MATZ wrote: [...] >> +/** Structure defining a mempool handler. */ > Later in the text, I suggested to rename rte_mempool_handler to > rte_mempool_ops. > I believe that it explains the purpose of this struct better. It > would improve > consistency in function names (the *_ext_* mark is very strange and > inconsistent). I agree. I've gone through all the code and renamed to rte_mempool_handler_ops. >>> Ok. I meant rte_mempool_ops because I find the word "handler" to be >>> redundant. >> I prefer the use of the word handler, unless others also have opinions >> either way? > Well, I think rte_mempool_ops is clear enough, and shorter, > so I'd vote for it. > OK, I've just changed it. It will be in next revision. :) Regards, Dave.
[dpdk-dev] [PATCH v5 1/3] mempool: support external handler
On 5/31/2016 9:53 AM, Jerin Jacob wrote:
> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
>> New mempool handlers will use rte_mempool_create_empty(),
>> rte_mempool_set_handler(),
>> then rte_mempool_populate_*(). These three functions are new to this
>> release, to no problem
> Having separate APIs for external pool-manager create is worrisome in
> application perspective. Is it possible to have rte_mempool_[xmem]_create
> for the both external and existing SW pool manager and make
> rte_mempool_create_empty and rte_mempool_populate_* internal functions.
>
> IMO, We can do that by selecting specific rte_mempool_set_handler()
> based on _flags_ encoding, something like below
>
> bit 0 - 16 // generic bits uses across all the pool managers
> bit 16 - 23 // pool handler specific flags bits
> bit 24 - 31 // to select the specific pool manager(Up to 256 different
> flavors of
> pool managers, For backward compatibility, make '0'(in 24-31) to select
> existing SW pool manager.
>
> and applications can choose the handlers by selecting the flag in
> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
> applications to choose the pool handler from command line etc in future.
There might be issues with the 8-bit handler number, as we'd have to add
an api call to
first get the index of a given hander by name, then OR it into the
flags. That would mean
also extra API calls for the non-default external handlers. I do agree
with the handler-specific
bits though.
Having the _empty and _set_handler APIs seems to me to be OK for the
moment. Maybe Olivier could comment?
> and we can remove "mbuf: get default mempool handler from configuration"
> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
>
> What do you think?
The "configuration" patch is to allow users to quickly change the
mempool handler
by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
handler. It could just as easily be left out and use the
rte_mempool_create.
>> to add a parameter to one of them for the config data. Also since we're
>> adding some new
>> elements to the mempool structure, how about we add a new pointer for a void
>> pointer to a
>> config data structure, as defined by the handler.
>>
>> So, new element in rte_mempool struct alongside the *pool
>> void *pool;
>> void *pool_config;
>>
>> Then add a param to the rte_mempool_set_handler function:
>> int
>> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
>> *pool_config)
> IMO, Maybe we need to have _set_ and _get_.So I think we can have
> two separate callback in external pool-manger for that if required.
> IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
> for the configuration as mentioned above and add the new callbacks for
> set and get when required?
OK, We'll keep the config to the 8 bits of the flags for now. That will
also mean I won't
add the pool_config void pointer either (for the moment)
>>> 2) IMO, It is better to change void *pool in struct rte_mempool to
>>> anonymous union type, something like below, so that mempool
>>> implementation can choose the best type.
>>> union {
>>> void *pool;
>>> uint64_t val;
>>> }
>> Could we do this by using the union for the *pool_config suggested above,
>> would that give
>> you what you need?
> It would be an extra overhead for external pool manager to _alloc_ memory
> and store the allocated pointer in mempool struct(as *pool) and use pool for
> pointing other data structures as some implementation need only
> limited bytes to store the external pool manager specific context.
>
> In order to fix this problem, We may classify fast path and slow path
> elements in struct rte_mempool and move all fast path elements in first
> cache line and create an empty opaque space in the remaining bytes in the
> cache line so that if the external pool manager needs only limited space
> then it is not required to allocate the separate memory to save the
> per core cache in fast-path
>
> something like below,
> union {
> void *pool;
> uint64_t val;
> uint8_t extra_mem[16] // available free bytes in fast path cache line
>
> }
Something for the future, perhaps? Will the 8-bits in the flags suffice
for now?
> Other points,
>
> 1) Is it possible to remove unused is_mp in __mempool_put_bulk
> function as it is just a internal function.
Fixed
> 2) Considering "get" and "put" are the fast-path
[dpdk-dev] [dpdk-dev,v5,1/3] mempool: support external handler
On 5/31/2016 1:06 PM, Jan Viktorin wrote:
> On Tue, 31 May 2016 10:09:42 +0100
> "Hunt, David" wrote:
>
>> Hi Jan,
>>
> [...]
>
>>>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>>>> +
>>>> +/** Free the external pool. */
>>> What is the purpose of this callback?
>>> What exactly does it allocate?
>>> Some rte_mempool internals?
>>> Or the memory?
>>> What does it return?
>> This is the main allocate function of the handler. It is up to the
>> mempool handlers control.
>> The handler's alloc function does whatever it needs to do to grab memory
>> for this handler, and places
>> a pointer to it in the *pool opaque pointer in the rte_mempool struct.
>> In the default handler, *pool
>> points to a ring, in other handlers, it will mostlikely point to a
>> different type of data structure. It will
>> be transparent to the application programmer.
> Thanks for explanation. Please, add doc comments there.
>
> Should the *mp be const here? It is not clear to me whether the callback is
> expected to modify the mempool or not (eg. set the pool pointer).
Comment added. Not const, as the *pool is set.
>>>> +typedef void (*rte_mempool_free_t)(void *p);
>>>> +
>>>> +/** Put an object in the external pool. */
>>> Why this *_free callback does not accept the rte_mempool param?
>>>
>> We're freeing the pool opaque data here.
> Add doc comments...
Done.
>>
>>>> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table,
>>>> unsigned n);
>>> What is the *p pointer?
>>> What is the obj_table?
>>> Why is it void *?
>>> Why is it const?
>>>
>> The *p pointer is the opaque data for a given mempool handler (ring,
>> array, linked list, etc)
> Again, doc comments...
>
> I don't like the obj_table representation to be an array of void *. I could
> see
> it already in DPDK for defining Ethernet driver queues, so, it's probably not
> an issue. I just say, I would prefer some basic type safety like
>
> struct rte_mempool_obj {
> void *p;
> };
>
> Is there somebody with different opinions?
>
> [...]
Comments added. I've left as a void* for the moment.
>>>> +
>>>> +/** Structure defining a mempool handler. */
>>> Later in the text, I suggested to rename rte_mempool_handler to
>>> rte_mempool_ops.
>>> I believe that it explains the purpose of this struct better. It would
>>> improve
>>> consistency in function names (the *_ext_* mark is very strange and
>>> inconsistent).
>> I agree. I've gone through all the code and renamed to
>> rte_mempool_handler_ops.
> Ok. I meant rte_mempool_ops because I find the word "handler" to be redundant.
I prefer the use of the word handler, unless others also have opinions
either way?
>
>>>> +/**
>>>> + * Set the handler of a mempool
>>>> + *
>>>> + * This can only be done on a mempool that is not populated, i.e. just
>>>> after
>>>> + * a call to rte_mempool_create_empty().
>>>> + *
>>>> + * @param mp
>>>> + * Pointer to the memory pool.
>>>> + * @param name
>>>> + * Name of the handler.
>>>> + * @return
>>>> + * - 0: Sucess; the new handler is configured.
>>>> + * - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>>>
>>> The body of rte_mempool_set_handler does not set errno at all.
>>> It returns e.g. -EEXIST.
>> This is up to the handler. We cannot know what codes will be returned at
>> this stage.
>> errno was a cut-and paste error, this is now fixed.
> I don't think so. The rte_mempool_set_handler is not handler-specific:
[...]
> So, it is possible to define those in the doc comment.
Ah, I see now. My mistake, I assumed a different function. Added now.
>>
>>>> + */
>>>> +int
>>>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
>>>> +
>>>> +/**
>>>> + * Register an external pool handler.
>>>> + *
>>>> + * @param h
>>>> + * Pointer to the external pool handler
>>>> + * @return
>>>> + * - >=0: Sucess; return the index of the handler in the table.
>>>> + * - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>> This is up to the handler. We cannot know what codes will be
[dpdk-dev] [dpdk-dev, v5, 3/3] mbuf: get default mempool handler from configuration
On 5/23/2016 1:40 PM, Jan Viktorin wrote: > On Thu, 19 May 2016 14:45:01 +0100 > David Hunt wrote: --snip-- >> +mp = rte_mempool_create_empty(name, n, elt_size, cache_size, >> + sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); >> +if (mp == NULL) >> +return NULL; >> + >> +rte_mempool_set_handler(mp, RTE_MBUF_DEFAULT_MEMPOOL_HANDLER); > Check for a failure is missing here. Especially -EEXIST. Done. --snip-- Thanks, Dave.
[dpdk-dev] [dpdk-dev, v5, 2/3] app/test: test external mempool handler
Hi Jan,
On 5/23/2016 1:45 PM, Jan Viktorin wrote:
> On Thu, 19 May 2016 14:45:00 +0100
> David Hunt wrote:
--snip--
>> + * Loop though all the element pointers and allocate a chunk of memory, then
> s/though/through/
Fixed.
>> +static struct rte_mempool_handler mempool_handler_custom = {
>> +.name = "custom_handler",
>> +.alloc = custom_mempool_alloc,
>> +.free = custom_mempool_free,
>> +.put = custom_mempool_put,
>> +.get = custom_mempool_get,
>> +.get_count = custom_mempool_get_count,
>> +};
>> +
>> +MEMPOOL_REGISTER_HANDLER(mempool_handler_custom);
> What about to drop the rte_mempool_handler.name field and derive the
> name from the variable name given to the MEMPOOL_REGISTER_HANDLER.
> The MEMPOOL_REGISTER_HANDLER sould do some macro magic inside and call
>
>rte_mempool_handler_register(name, handler);
>
> Just an idea...
Lets see if anyone else has any strong opinions on this :)
>> +
>> +/*
>>* save the object number in the first 4 bytes of object data. All
>>* other bytes are set to 0.
>>*/
>> @@ -479,6 +569,7 @@ test_mempool(void)
>> {
>> struct rte_mempool *mp_cache = NULL;
>> struct rte_mempool *mp_nocache = NULL;
>> +struct rte_mempool *mp_ext = NULL;
>>
>> rte_atomic32_init();
>>
>> @@ -507,6 +598,27 @@ test_mempool(void)
>> goto err;
>> }
>>
>> +/* create a mempool with an external handler */
>> +mp_ext = rte_mempool_create_empty("test_ext",
>> +MEMPOOL_SIZE,
>> +MEMPOOL_ELT_SIZE,
>> +RTE_MEMPOOL_CACHE_MAX_SIZE, 0,
>> +SOCKET_ID_ANY, 0);
>> +
>> +if (mp_ext == NULL) {
>> +printf("cannot allocate mp_ext mempool\n");
>> +goto err;
>> +}
>> +if (rte_mempool_set_handler(mp_ext, "custom_handler") < 0) {
>> +printf("cannot set custom handler\n");
>> +goto err;
>> +}
>> +if (rte_mempool_populate_default(mp_ext) < 0) {
>> +printf("cannot populate mp_ext mempool\n");
>> +goto err;
>> +}
>> +rte_mempool_obj_iter(mp_ext, my_obj_init, NULL);
>> +
> The test becomes quite complex. What about having several smaller
> tests with a clear setup and cleanup steps?
I guess that's something we can look at in the future. For the moment
can we leave it?
Thanks,
Dave.
[dpdk-dev] [dpdk-dev,v5,1/3] mempool: support external handler
Hi Jan,
On 5/23/2016 1:35 PM, Jan Viktorin wrote:
>> Until now, the objects stored in mempool mempool were internally stored a
> s/mempool mempool/mempool/
>
> stored _in_ a ring?
Fixed.
>
> @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
> n_get_bulk);
> if (unlikely(ret < 0)) {
> rte_mempool_dump(stdout, mp);
> - rte_ring_dump(stdout, mp->ring);
> /* in this case, objects are lost... */
> return -1;
> }
> I think, this should be in a separate patch explaining the reason to remove
> it.
Done. Moved.
>> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
>> index 43423e0..f19366e 100644
>> --- a/lib/librte_mempool/Makefile
>> +++ b/lib/librte_mempool/Makefile
>> @@ -42,6 +42,8 @@ LIBABIVER := 2
>>
>> # all source are stored in SRCS-y
>> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_default.c
>> # install includes
>> SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c
>> b/lib/librte_mempool/rte_mempool.c
>> index 1ab6701..6ec2b3f 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj,
>> phys_addr_t physaddr)
>> #endif
>>
>> /* enqueue in ring */
>> -rte_ring_sp_enqueue(mp->ring, obj);
>> +rte_mempool_ext_put_bulk(mp, , 1);
> I suppose this is OK, however, replacing "enqueue" by "put" (semantically)
> sounds to me
> like a bug. Enqueue is put into a queue. Put is to drop a reference.
Yes, Makes sense. Changed 'put' and 'get' to 'enqueue' and 'dequeue'
>
>>
>> -/* create the internal ring */
>> -static int
>> -rte_mempool_ring_create(struct rte_mempool *mp)
>> -{
>> -int rg_flags = 0, ret;
>> -char rg_name[RTE_RING_NAMESIZE];
>> -struct rte_ring *r;
>> -
>> -ret = snprintf(rg_name, sizeof(rg_name),
>> -RTE_MEMPOOL_MZ_FORMAT, mp->name);
>> -if (ret < 0 || ret >= (int)sizeof(rg_name))
>> -return -ENAMETOOLONG;
>> -
>> -/* ring flags */
>> -if (mp->flags & MEMPOOL_F_SP_PUT)
>> -rg_flags |= RING_F_SP_ENQ;
>> -if (mp->flags & MEMPOOL_F_SC_GET)
>> -rg_flags |= RING_F_SC_DEQ;
>> -
>> -/* Allocate the ring that will be used to store objects.
>> - * Ring functions will return appropriate errors if we are
>> - * running as a secondary process etc., so no checks made
>> - * in this function for that condition.
>> - */
>> -r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
>> -mp->socket_id, rg_flags);
>> -if (r == NULL)
>> -return -rte_errno;
>> -
>> -mp->ring = r;
>> -mp->flags |= MEMPOOL_F_RING_CREATED;
>> -return 0;
>> -}
> This is a big change. I suggest (if possible) to make a separate patch with
> something like "replace rte_mempool_ring_create by ...". Where is this code
> placed now?
The code is not gone away, it's now part of the default handler, which
uses a ring. It's
in rte_mempool_default.c
>> -
>> /* free a memchunk allocated with rte_memzone_reserve() */
>> static void
>> rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr
>> *memhdr,
>> @@ -351,7 +317,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
>> void *elt;
>>
>> while (!STAILQ_EMPTY(>elt_list)) {
>> -rte_ring_sc_dequeue(mp->ring, );
>> +rte_mempool_ext_get_bulk(mp, , 1);
> Similar as for put_bulk... Replacing "dequeue" by "get" (semantically) sounds
> to me
> like a bug. Dequeue is drop from a queue. Get is to obtain a reference.
Done
>> (void)elt;
>> STAILQ_REMOVE_HEAD(>elt_list, next);
>> mp->populated_size--;
>> @@ -380,15 +346,18 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char
>> *vaddr,
>> unsigned i = 0;
>> size_t off;
>> struct rte_mempool_memhdr *memhdr;
>> -int ret;
>>
>> /* create the internal ring if not already done */
>> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
>> -ret = rte_mempool_ring_create(mp);
>> -if (ret < 0)
>> -return ret;
>> +rte_errno = 0;
>> +mp->pool = rte_mempool_ext_alloc(mp);
>> +if (mp->pool == NULL) {
>> +if (rte_errno == 0)
>> +return -EINVAL;
>> +else
>> +return -rte_errno;
>> +}
>> }
>> -
> Is this a whitespace change?
Accidental. Reverted
>> +
>> +/*
>> +
[dpdk-dev] [PATCH v5 1/3] mempool: support external handler
On 5/27/2016 11:33 AM, Jerin Jacob wrote: > On Fri, May 27, 2016 at 10:52:42AM +0100, Hunt, David wrote: >> >> On 5/24/2016 4:35 PM, Jerin Jacob wrote: >>> On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote: >>>> + /* >>>> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to >>>> + * set the correct index into the handler table. >>>> + */ >>>> + if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) >>>> + rte_mempool_set_handler(mp, "ring_sp_sc"); >>>> + else if (flags & MEMPOOL_F_SP_PUT) >>>> + rte_mempool_set_handler(mp, "ring_sp_mc"); >>>> + else if (flags & MEMPOOL_F_SC_GET) >>>> + rte_mempool_set_handler(mp, "ring_mp_sc"); >>>> + else >>>> + rte_mempool_set_handler(mp, "ring_mp_mc"); >>> IMO, We should decouple the implementation specific flags of _a_ >>> external pool manager implementation from the generic >>> rte_mempool_create_empty >>> function as going further when we introduce new flags for custom HW >>> accelerated >>> external pool manager then this common code will be bloated. >> These flags are only there to maintain backward compatibility for the >> default handlers. I would not >> envisage adding more flags to this, I would suggest just adding a new >> handler using the new API calls. >> So I would not see this code growing much in the future. > IMHO, For _each_ HW accelerated external pool manager we may need to introduce > specific flag to tune to specific use cases.i.e MEMPOOL_F_* flags for > this exiting pool manager implemented in SW. For instance, when we add > a new HW external pool manager we may need to add > MEMPOOL_MYHW_DONT_FREE_ON_SEND > (just a random name) to achieve certain functionally. > > So I propose let "unsigned flags" in mempool create to be the opaque type and > each > external pool manager can define what it makes sense to that specific > pool manager as there is NO other means to configure the pool manager. > > For instance, on HW accelerated pool manager, the flag MEMPOOL_F_SP_PUT may > not make much sense as it can work with MP without any additional > settings in HW. > > So instead of adding these checks in common code, IMO, lets move this > to a pool manager specific "function pointer" function and invoke > the function pointer from generic mempool create function. > > What do you think? > > Jerin Jerin, That chunk of code above would be better moved all right. I'd suggest moving it to the rte_mempool_create function, as that's the one that needs the backward compatibility. On the flags issue, each mempool handler can re-interpret the flags as needed. Maybe we could use the upper half of the bits for different handlers, changing the meaning of the bits depending on which handler is being set up. We can then keep the lower half for bits that are common across all handlers? That way the user can just set the bits they are interested in for that handler. Also, the alloc function has access to the flags, so maybe the handler specific setup could be handled in the alloc function rather than adding a new function pointer? Of course, that won't help if we need to pass in more data, in which case we'd probably need an opaque data pointer somewhere. It would probably be most useful to pass it in with the alloc, which may need the data. Any suggestions? Regards, Dave.
[dpdk-dev] [PATCH] mbuf: replace c memcpy code semantics with optimized rte_memcpy
On 5/27/2016 11:24 AM, Hunt, David wrote: > > > On 5/24/2016 4:17 PM, Jerin Jacob wrote: >> On Tue, May 24, 2016 at 04:59:47PM +0200, Olivier Matz wrote: >> >>> Are you seeing some performance improvement by using rte_memcpy()? >> Yes, In some case, In default case, It was replaced with memcpy by the >> compiler itself(gcc 5.3). But when I tried external mempool manager >> patch and >> then performance dropped almost 800Kpps. Debugging further it turns >> out that >> external mempool managers unrelated change was knocking out the memcpy. >> explicit rte_memcpy brought back 500Kpps. Remaing 300Kpps drop is still >> unknown(In my test setup, packets are in the local cache, so it must be >> something do with __mempool_put_bulk text alignment change or similar. >> >> Anyone else observed performance drop with external poolmanager? >> >> Jerin > > Jerin, > I'm seeing a 300kpps drop in throughput when I apply this on top > of the external > mempool manager patch. If you're seeing an increase if you apply this > patch first, then > a drop when applying the mempool manager, the two patches must be > conflicting in > some way. We probably need to investigate further. > Regards, > Dave. > On further investigation, I now have a setup with no performance degradation. My previous tests were accessing the NICS on a different NUMA node. Once I initiated testPMD with the correct coremask, the difference between pre and post rte_memcpy patch is negligible (maybe 0.1% drop). Regards, Dave.
[dpdk-dev] [PATCH] mbuf: replace c memcpy code semantics with optimized rte_memcpy
On 5/24/2016 4:17 PM, Jerin Jacob wrote: > On Tue, May 24, 2016 at 04:59:47PM +0200, Olivier Matz wrote: > >> Are you seeing some performance improvement by using rte_memcpy()? > Yes, In some case, In default case, It was replaced with memcpy by the > compiler itself(gcc 5.3). But when I tried external mempool manager patch and > then performance dropped almost 800Kpps. Debugging further it turns out that > external mempool managers unrelated change was knocking out the memcpy. > explicit rte_memcpy brought back 500Kpps. Remaing 300Kpps drop is still > unknown(In my test setup, packets are in the local cache, so it must be > something do with __mempool_put_bulk text alignment change or similar. > > Anyone else observed performance drop with external poolmanager? > > Jerin Jerin, I'm seeing a 300kpps drop in throughput when I apply this on top of the external mempool manager patch. If you're seeing an increase if you apply this patch first, then a drop when applying the mempool manager, the two patches must be conflicting in some way. We probably need to investigate further. Regards, Dave.
[dpdk-dev] [PATCH v5 1/3] mempool: support external handler
On 5/24/2016 4:35 PM, Jerin Jacob wrote:
> On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
>> +/*
>> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>> + * set the correct index into the handler table.
>> + */
>> +if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>> +rte_mempool_set_handler(mp, "ring_sp_sc");
>> +else if (flags & MEMPOOL_F_SP_PUT)
>> +rte_mempool_set_handler(mp, "ring_sp_mc");
>> +else if (flags & MEMPOOL_F_SC_GET)
>> +rte_mempool_set_handler(mp, "ring_mp_sc");
>> +else
>> +rte_mempool_set_handler(mp, "ring_mp_mc");
> IMO, We should decouple the implementation specific flags of _a_
> external pool manager implementation from the generic rte_mempool_create_empty
> function as going further when we introduce new flags for custom HW
> accelerated
> external pool manager then this common code will be bloated.
These flags are only there to maintain backward compatibility for the
default handlers. I would not
envisage adding more flags to this, I would suggest just adding a new
handler using the new API calls.
So I would not see this code growing much in the future.
>> +/** Structure storing the table of registered handlers. */
>> +struct rte_mempool_handler_table {
>> +rte_spinlock_t sl; /**< Spinlock for add/delete. */
>> +uint32_t num_handlers; /**< Number of handlers in the table. */
>> +/** Storage for all possible handlers. */
>> +struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX];
>> +};
> add __rte_cache_aligned to this structure to avoid "handler" memory
> cacheline being shared with other variables
Will do.
>> +
>> +/** Array of registered handlers */
>> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
>> +
>> +/**
>> + * @internal Get the mempool handler from its index.
>> + *
>> + * @param handler_idx
>> + * The index of the handler in the handler table. It must be a valid
>> + * index: (0 <= idx < num_handlers).
>> + * @return
>> + * The pointer to the handler in the table.
>> + */
>> +static struct rte_mempool_handler *
> inline?
Will do.
>> /* How many do we require i.e. number to fill the cache + the
>> request */
>> -ret = rte_ring_mc_dequeue_bulk(mp->ring,
>> >objs[cache->len], req);
>> +ret = rte_mempool_ext_get_bulk(mp,
> This makes inline function to a function pointer. Nothing wrong in
> that. However, Do you see any performance drop with "local cache" only
> use case?
>
> http://dpdk.org/dev/patchwork/patch/12993/
With the latest mempool manager patch (without 12933), I see no
performance degradation on my Haswell machine.
However, when I apply patch 12933, I'm seeing a 200-300 kpps drop.
With 12933, the mempool_perf_autotest is showing 24% more
enqueues/dequeues, but testpmd forwarding
traffic between 2 40Gig interfaces from a hardware traffic generator
with one core doing the forwarding
is showing a drop of 200-300kpps.
Regards,
Dave.
---snip---
[dpdk-dev] [dpdk-dev,v5,1/3] mempool: support external handler
On 5/23/2016 1:35 PM, Jan Viktorin wrote: > Hello David, > > please, see my comments inline. > > I didn't see the previous versions of the mempool (well, only very roughly) > so I am > probably missing some points... My point of view is as a user of the handler > API. > I need to understand the API to implement a custom handler for my purposes. Thanks for the review, Jan. I'm working on the changes now, will post soon. I'll reply to each of you're emails when I'm ready with the patch. Regards, David.
[dpdk-dev] [PATCH v2 2/3] mempool: make declaration of handler structs const
On 5/23/2016 1:55 PM, Olivier Matz wrote: > Hi David, > > On 05/19/2016 04:48 PM, David Hunt wrote: >> For security, any data structure with function pointers should be const > I think this should be merged in the patchset adding the mempool > handler feature. Agreed, I'm working on that now, merging it in with some changes suggested by Jan on the other patch. Thanks, David.
[dpdk-dev] [PATCH 1/2] mempool: add stack (fifo) mempool handler
On 5/5/2016 10:28 PM, Stephen Hemminger wrote: > Overall, this is ok, but why can't it be the default? Backward compatibility would probably be the main reason, to have the least impact when recompiling. > Lots of little things should be cleaned up I've submitted a v2, and addressed all your issues. Thanks for the review. Regards, Dave. --snip---
[dpdk-dev] v2 mempool: add stack (lifo) mempool handler
On 5/19/2016 3:48 PM, David Hunt wrote: > This patch set adds a fifo stack handler to the external mempool > manager. Apologies, cut and paste error, should be lifo stack handler. I thought I'd caught all of these... > This patch set depends on the 3 part external mempool handler > patch set (v5 of the series): > http://dpdk.org/ml/archives/dev/2016-May/039364.html > > v2 changes: >* updated based on mailing list feedback (Thanks Stephen) >* checkpatch fixes. > >
[dpdk-dev] [PATCH] tools:new tool for system info CPU, memory and huge pages
Hi Keith.
Works nicely on the few different machines I tried it on.
Regards,
Dave.
On 5/13/2016 4:43 PM, Keith Wiles wrote:
> The new tool uses /sys/devices instead of /proc directory, which
> does not exist on all systems. If the procfs is not available
> then memory and huge page information is skipped.
>
> The tool also can emit a json format in short or long form to
> allow for machine readable information.
>
> Here is the usage information:
>
> Usage: sys_info.py [options]
>
> Show the lcores layout with core id and socket(s).
>
> Options:
> --help or -h:
> Display the usage information and quit
>
> --long or -l:
> Display the information in a machine readable long format.
>
> --short or -s:
> Display the information in a machine readable short format.
>
> --pci or -p:
> Display all of the Ethernet devices in the system using 'lspci'.
>
> --version or -v:
> Display the current version number of this script.
>
> --debug or -d:
> Output some debug information.
>
> default:
> Display the information in a human readable format.
>
> Signed-off-by: Keith Wiles
> ---
> tools/sys_info.py | 551
> ++
> 1 file changed, 551 insertions(+)
> create mode 100755 tools/sys_info.py
>
> diff --git a/tools/sys_info.py b/tools/sys_info.py
> new file mode 100755
> index 000..5e09d12
> --- /dev/null
> +++ b/tools/sys_info.py
> @@ -0,0 +1,551 @@
> +#! /usr/bin/python
> +#
> +# BSD LICENSE
> +#
> +# Copyright(c) 2016 Intel Corporation. All rights reserved.
> +# All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in
> +# the documentation and/or other materials provided with the
> +# distribution.
> +# * Neither the name of Intel Corporation nor the names of its
> +# contributors may be used to endorse or promote products derived
> +# from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +#
> +
> +import os, sys
> +import getopt
> +import json
> +from os.path import exists, abspath, dirname, basename
> +
> +version = "0.1.3"
> +
> +# Global lists and flags
> +machine_readable = 0
> +show_pci = False
> +debug_flag = False
> +coremaps_flag = False
> +
> +sys_info = {}
> +coremaps = {}
> +
> +def proc_cpuinfo_path():
> +'''Return the cpu information from /proc'''
> +return "/proc/cpuinfo"
> +
> +def proc_sysinfo_path():
> +'''Return the system path string from /proc'''
> +return "/proc/sysinfo"
> +
> +def proc_meminfo_path():
> +'''Return the memory information path from /proc'''
> +return "/proc/meminfo"
> +
> +def sys_system_path():
> +'''Return the system path string from /sys'''
> +return "/sys/devices/system"
> +
> +def read_file(path, whole_file=False):
> +'''Read the first line of a file'''
> +
> +if os.access(path, os.F_OK) == False:
> +print "Path (%s) Not found" % path
> +return ""
> +
> +fd = open(path)
> +if whole_file == True:
> +lines = fd.readlines()
> +else:
> +line = fd.readline()
> +fd.close()
> +
> +if whole_file == True:
> +return lines
> +
> +return line
> +
> +def get_range(line):
> +'''Split a line and convert to low/high values'''
> +
> +line = line.strip()
> +
> +if '-' in line:
> +low, high = line.split("-")
> +elif ',' in line:
> +low, high = line.split(",")
> +else:
> +return [int(line)]
> +
> +return [int(low), int(high)]
> +
> +def get_ranges(line):
> +'''Split a set of ranges into first low/high, second low/high value'''
> +
> +line = line.strip()
> +
> +first, second =
[dpdk-dev] [PATCH 0/2] mempool: add stack (fifo) mempool handler
On 5/6/2016 1:34 AM, Tan, Jianfeng wrote: > Hi David, > > > On 5/6/2016 2:29 AM, David Hunt wrote: >> This patch set adds a fifo stack handler to the external mempool >> manager. > > Just a minor confusion for me. Usually, we refer stack as LIFO and > queue as FIFO. So is there any particular reason why we call "stack > (fifo)" here? > > Thanks, > Jianfeng Jianfeng, You are correct. That is a typo. It should read LIFO. Regards, Dave.
[dpdk-dev] [PATCH 00/36] mempool: rework memory allocation
On 4/14/2016 3:01 PM, Olivier MATZ wrote: > > On 04/14/2016 03:50 PM, Wiles, Keith wrote: --snip-- >> I have not digested this complete patch yet, but this one popped out >> at me as the External Memory Manager support is setting in the wings >> for 16.07 release. If this causes the EMM patch to be rewritten or >> updated that seems like a problem to me. Does this patch add the >> External Memory Manager support? >> http://thread.gmane.org/gmane.comp.networking.dpdk.devel/32015/focus=35107 >> > > I've reworked the series you are referring to, and rebased it on top > of this series. Please see: > http://dpdk.org/ml/archives/dev/2016-April/037509.html > Thanks for your help on this, Olivier. Much appreciated. Regards, David.
[dpdk-dev] [PATCH] doc: announce ABI changes for user-owned mempool caches
On 4/5/2016 4:42 PM, Olivier Matz wrote: > On 04/05/2016 11:23 AM, Lazaros Koromilas wrote: >> Deprecation notice for 16.04 for changes targeting release 16.07. >> The changes affect struct rte_mempool, rte_mempool_cache and the >> mempool API. >> >> Signed-off-by: Lazaros Koromilas > Acked-by: Olivier Matz > Acked-by: David Hunt
[dpdk-dev] [PATCH] doc: mempool ABI deprecation notice for 16.07
On 4/4/2016 3:38 PM, Thomas Monjalon wrote: > 2016-03-17 10:05, Olivier Matz: >> Add a deprecation notice for coming changes in mempool for 16.07. > [...] >> +* librte_mempool: new fixes and features will be added in 16.07: >> + allocation of large mempool in several virtual memory chunks, new API >> + to populate a mempool, new API to free a mempool, allocation in >> + anonymous mapping, drop of specific dom0 code. These changes will >> + induce a modification of the rte_mempool structure, plus a >> + modification of the API of rte_mempool_obj_iter(), implying a breakage >> + of the ABI. > Acked-by: Thomas Monjalon > > Other people involved in the discussion wanting to bring their support? Acked-by: David Hunt Regards, David.
[dpdk-dev] [PATCH v5 4/4] ena: DPDK polling-mode driver for Amazon Elastic Network Adapters (ENA)
Hi Jan, On 3/7/2016 8:05 PM, Jan Medala wrote: --snip-- > diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h > new file mode 100644 > index 000..7f069b9 > --- /dev/null > +++ b/drivers/net/ena/ena_ethdev.h > @@ -0,0 +1,154 @@ > +/*- > +* BSD LICENSE > +* > +* Copyright (c) 2015-2016 Amazon.com, Inc. or its affiliates. > +* All rights reserved. > +* > +* Redistribution and use in source and binary forms, with or without > +* modification, are permitted provided that the following conditions > +* are met: > +* > +* * Redistributions of source code must retain the above copyright > +* notice, this list of conditions and the following disclaimer. > +* * Redistributions in binary form must reproduce the above copyright > +* notice, this list of conditions and the following disclaimer in > +* the documentation and/or other materials provided with the > +* distribution. > +* * Neither the name of copyright holder nor the names of its > +* contributors may be used to endorse or promote products derived > +* from this software without specific prior written permission. > +* > +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > +* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > +* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > +* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > +* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > +* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > +* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > +* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > +* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > +* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > +*/ > + > +#ifndef _ENA_ETHDEV_H_ > +#define _ENA_ETHDEV_H_ > + > +#include > + > +#pragma GCC diagnostic ignored "-Wcast-qual" Is this #pragma really needed here? My setup compiles fine without warnings, and I reckon it's better not have this line if it's not needed just in case we inadvertently introduce additional warnings in the future. ---snip-- The only other thing I'd note with the rest of the patch-set is the output from checkpatch, which Bruce has already mentioned. Regards, David.
[dpdk-dev] [PATCH v5 1/4] ena: Amazon ENA documentation
On 3/7/2016 8:05 PM, Jan Medala wrote: > Signed-off-by: Evgeny Schemeilin > Signed-off-by: Alexander Matushevsky > Signed-off-by: Jan Medala > Signed-off-by: Jakub Palider > --- > MAINTAINERS | 8 ++ > doc/guides/nics/ena.rst | 252 > ++ > doc/guides/nics/index.rst | 1 + > 3 files changed, 261 insertions(+) > create mode 100644 doc/guides/nics/ena.rst --snip-- Small nit while doing 'git am': Applying: ena: Amazon ENA documentation /root/dpdk/.git/rebase-apply/patch:197: trailing whitespace. Any Linux distribution fulfilling the conditions described in ``System Requirements`` /root/dpdk/.git/rebase-apply/patch:284: new blank line at EOF. + warning: 2 lines add whitespace errors. Regards, David.
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi Olivier, On 3/9/2016 4:31 PM, Olivier MATZ wrote: > Hi David, > > On 03/09/2016 05:28 PM, Hunt, David wrote: > >> Sure, v4 will remove the NEXT_ABI patch , and replace it with just the >> ABI break announcement for 16.07. For anyone who what's to try out the >> patch, they can always get it from patchwork, but not as part 16.04. > I think it's better to have the deprecation notice in a separate > mail, outside of the patch series, so Thomas can just apply this > one and let the series pending for 16.07. > > Thanks, > Olivier Yes, sure, makes perfect sense. Thanks, David.
[dpdk-dev] [PATCH v3 3/4] mempool: allow rte_pktmbuf_pool_create switch between memool handlers
Hi Panu, On 3/9/2016 10:54 AM, Panu Matilainen wrote: > On 03/09/2016 11:50 AM, David Hunt wrote: >> If the user wants to have rte_pktmbuf_pool_create() use an external >> mempool >> handler, they define RTE_MEMPOOL_HANDLER_NAME to be the name of the >> mempool handler they wish to use, and change RTE_MEMPOOL_HANDLER_EXT >> to 'y' >> >> Signed-off-by: David Hunt >> --- >> config/common_base | 2 ++ >> lib/librte_mbuf/rte_mbuf.c | 8 >> 2 files changed, 10 insertions(+) >> >> diff --git a/config/common_base b/config/common_base >> index 1af28c8..9d70cf4 100644 >> --- a/config/common_base >> +++ b/config/common_base >> @@ -350,6 +350,8 @@ CONFIG_RTE_RING_PAUSE_REP_COUNT=0 >> CONFIG_RTE_LIBRTE_MEMPOOL=y >> CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE=512 >> CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n >> +CONFIG_RTE_MEMPOOL_HANDLER_EXT=n >> +CONFIG_RTE_MEMPOOL_HANDLER_NAME="custom_handler" >> >> # >> # Compile librte_mbuf >> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c >> index c18b438..42b0cd1 100644 >> --- a/lib/librte_mbuf/rte_mbuf.c >> +++ b/lib/librte_mbuf/rte_mbuf.c >> @@ -167,10 +167,18 @@ rte_pktmbuf_pool_create(const char *name, >> unsigned n, >> mbp_priv.mbuf_data_room_size = data_room_size; >> mbp_priv.mbuf_priv_size = priv_size; >> >> +#ifdef RTE_MEMPOOL_HANDLER_EXT >> +return rte_mempool_create_ext(name, n, elt_size, >> +cache_size, sizeof(struct rte_pktmbuf_pool_private), >> +rte_pktmbuf_pool_init, _priv, rte_pktmbuf_init, NULL, >> +socket_id, 0, >> +RTE_MEMPOOL_HANDLER_NAME); >> +#else >> return rte_mempool_create(name, n, elt_size, >> cache_size, sizeof(struct rte_pktmbuf_pool_private), >> rte_pktmbuf_pool_init, _priv, rte_pktmbuf_init, NULL, >> socket_id, 0); >> +#endif >> } >> >> /* do some sanity checks on a mbuf: panic if it fails */ >> > > This kind of thing really has to be run-time configurable, not a > library build-time option. > > - Panu - Interesting point. I was attempting to minimise the amount of application code changes. Would you prefer if I took out that change, and added a new rte_pktmbuf_pool_create_ext() function which tool an extra parameter as the mempool handler name to use? /* helper to create a mbuf pool using external mempool handler */ struct rte_mempool * rte_pktmbuf_pool_create_ext(const char *name, unsigned n, unsigned cache_size, uint16_t priv_size, uint16_t data_room_size, int socket_id, const char *handler_name) That way we could leave the old rte_pktmbuf_pool_create() exactly as it is, and any apps that wanted to use an external handler could call rte_pktmbuf_pool_create_ext() I could do this easily enough for v4 (which I hope to get out later today)? Thanks, David.
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi Panu, On 3/9/2016 10:46 AM, Panu Matilainen wrote: > On 03/09/2016 11:50 AM, David Hunt wrote: >> This patch is for those people who want to be easily able to switch >> between the new mempool layout and the old. Change the value of >> RTE_NEXT_ABI in common_base config file > > I guess the idea here is to document how to switch between the ABIs > but to me this reads as if this patch is supposed to change the value > in common_base. Of course there's no such change included (nor should > there be) here, but the description could use some fine-tuning perhaps. > You're right, I'll clarify the comments. v4 due soon. >> >> v3: Updated to take re-work of file layouts into consideration >> >> v2: Kept all the NEXT_ABI defs to this patch so as to make the >> previous patches easier to read, and also to imake it clear what >> code is necessary to keep ABI compatibility when NEXT_ABI is >> disabled. > > Maybe its just me, but: > I can see why NEXT_ABI is in a separate patch for review purposes but > for final commit this split doesn't seem right to me. In any case its > quite a large change for NEXT_ABI. > The patch basically re-introduces the old (pre-mempool) code as the refactoring of the code would have made the NEXT_ABI additions totally unreadable. I think this way is the lesser of two evils. > In any case, you should add a deprecation notice for the oncoming ABI > break in 16.07. > Sure, I'll add that in v4. > - Panu - > Thanks for the comments, Regards, David.
[dpdk-dev] [PATCH v3 0/4] external mempool manager
On 3/9/2016 9:50 AM, David Hunt wrote: > * removed stack hanler, may re-introduce at a later date > Some comments regarding this have made good points to keep this handler in. Will do in v4. Regards, David.
[dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support
Hi Olivier,
On 3/4/2016 9:05 AM, Olivier MATZ wrote:
> Hi David,
>
@@ -622,6 +607,10 @@ rte_mempool_xmem_create(const char *name,
unsigned n, unsigned elt_size,
mp->elt_va_end = mp->elt_va_start;
+/* Parameters are setup. Call the mempool handler alloc */
+if ((rte_mempool_ext_alloc(mp, name, n, socket_id, flags)) == NULL)
+goto exit;
+
>>> I think some memory needs to be freed here. At least 'te'.
>> Done in v2
> Please note that in the meanwhile, this fix has been pushed (as we need
> it for next release):
> http://dpdk.org/browse/dpdk/commit/lib/librte_mempool/rte_mempool.c?id=86f36ff9578b5f3d697c8fcf6072dcb70e2b246f
v3 will be rebased on top of the latest head of the repo.
diff --git a/lib/librte_mempool/rte_mempool.h
b/lib/librte_mempool/rte_mempool.h
index 6e2390a..620cfb7 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -88,6 +88,8 @@ extern "C" {
struct rte_mempool_debug_stats {
uint64_t put_bulk; /**< Number of puts. */
uint64_t put_objs; /**< Number of objects successfully
put. */
+uint64_t put_pool_bulk;/**< Number of puts into pool. */
+uint64_t put_pool_objs;/**< Number of objects into pool. */
uint64_t get_success_bulk; /**< Successful allocation number. */
uint64_t get_success_objs; /**< Objects successfully allocated. */
uint64_t get_fail_bulk;/**< Failed allocation number. */
>>> I think the comment of put_pool_objs is not very clear.
>>> Shouldn't we have the same stats for get?
>>>
>> Not used, removed. Covered by put_bulk.
> I guess you mean it will be removed in v3? It is still there in the v2
> (the field, not the comment that has been fixed).
>
> Shouldn't we have the same stats for get?
I believe get's are covered by the get_success_bulk and get_fail_bulk
/**
* The RTE mempool structure.
*/
struct rte_mempool {
char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
-struct rte_ring *ring; /**< Ring to store objects. */
phys_addr_t phys_addr; /**< Phys. addr. of mempool
struct. */
int flags; /**< Flags of the mempool. */
uint32_t size; /**< Size of the mempool. */
@@ -194,6 +270,11 @@ struct rte_mempool {
unsigned private_data_size; /**< Size of private data. */
+/* Common pool data structure pointer */
+void *rt_pool __rte_cache_aligned;
>>> What is the meaning of rt_pool?
>> I agree that it's probably not a very good name. Since it's basically
>> the pointer which is used by the handlers callbacks, maybe we should
>> call it mempool_storage? That leaves it generic enough that it can point
>> at a ring, an array, or whatever else is needed for a particular handler.
> My question was more about the "rt_" prefix. Maybe I missed something
> obvious? I think "pool" or "pool_handler" is ok.
v3 will use "pool"
-
+
+/* add new handler index */
+handler_idx = mempool_handler_list.num_handlers++;
+
+snprintf(mempool_handler_list.handler[handler_idx].name,
+RTE_MEMPOOL_NAMESIZE, "%s", h->name);
+mempool_handler_list.handler[handler_idx].alloc = h->alloc;
+mempool_handler_list.handler[handler_idx].put = h->put;
+mempool_handler_list.handler[handler_idx].get = h->get;
+mempool_handler_list.handler[handler_idx].get_count = h->get_count;
+
+rte_spinlock_unlock(_handler_list.sl);
+
+return handler_idx;
+}
>>> Why not using a similar mechanism than what we have for PMDs?
>>>
>>> void rte_eal_driver_register(struct rte_driver *driver)
>>> {
>>> TAILQ_INSERT_TAIL(_driver_list, driver, next);
>>> }
>>>
>> Already discussed previously, index needed over pointer because of
>> secondary processes.
> Could you add a comment stating this? It may help for next readers
> to have this info.
v3: Comment added to the header file where we define header_idx
explaining the use of an index versus pointers
>>> Last thing, I think this code should go in rte_mempool.c, not in
>>> rte_mempool_default.c.
>> I was trying to keep the default handlers together in their own file,
>> rather than having them in with the mempool framework. I think it's
>> better having them separate, and new handlers can go in their own files
>> also. no?
> OK for the following functions:
> common_ring_mp_put()
> common_ring_sp_put()
> common_ring_mc_get()
> common_ring_sc_get()
> common_ring_get_count()
> rte_mempool_common_ring_alloc() (note: only this one has
> a rte_mempool prefix, maybe it should be fixed)
>
> The other functions are part of the framework to add an external
> handler, I don't
[dpdk-dev] [PATCH] doc: deprecation notice in 16.04 for rte_mempool changes
Hi Keith, Here's another ack for this patch, in case it's needed. On 2/15/2016 9:20 AM, Olivier MATZ wrote: > Hi Keith, > > On 02/12/2016 07:38 PM, Keith Wiles wrote: >> Deprecation notice for 16.04 for changes to occur in >> release 16.07 for rte_mempool memory reduction. >> >> Signed-off-by: Keith Wiles > Acked-by: Olivier Matz Acked-by: David Hunt
[dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support
Olivier,
Here's my comments on your feedback. Hopefully I've covered all of
it this time, and I've summarised the outstanding questions at the bottom.
On 2/4/2016 2:52 PM, Olivier MATZ wrote:
>
>> -#ifndef RTE_LIBRTE_XEN_DOM0
>> -/* stub if DOM0 support not configured */
>> -struct rte_mempool *
>> -rte_dom0_mempool_create(const char *name __rte_unused,
>> -unsigned n __rte_unused,
>> -unsigned elt_size __rte_unused,
>> -unsigned cache_size __rte_unused,
>> -unsigned private_data_size __rte_unused,
>> -rte_mempool_ctor_t *mp_init __rte_unused,
>> -void *mp_init_arg __rte_unused,
>> -rte_mempool_obj_ctor_t *obj_init __rte_unused,
>> -void *obj_init_arg __rte_unused,
>> -int socket_id __rte_unused,
>> -unsigned flags __rte_unused)
>> -{
>> -rte_errno = EINVAL;
>> -return NULL;
>> -}
>> -#endif
>> -
>
> Could we move this is a separated commit?
> "mempool: remove unused rte_dom0_mempool_create stub"
Will do for v3.
--snip--
> return rte_mempool_xmem_create(name, n, elt_size,
>> - cache_size, private_data_size,
>> - mp_init, mp_init_arg,
>> - obj_init, obj_init_arg,
>> - socket_id, flags,
>> - NULL, NULL, MEMPOOL_PG_NUM_DEFAULT,
>> - MEMPOOL_PG_SHIFT_MAX);
>> +cache_size, private_data_size,
>> +mp_init, mp_init_arg,
>> +obj_init, obj_init_arg,
>> +socket_id, flags,
>> +NULL, NULL,
>> +MEMPOOL_PG_NUM_DEFAULT, MEMPOOL_PG_SHIFT_MAX);
>> }
>
> As far as I can see, you are not modifying the code here, only the
> style. For better readability, it should go in another commit that
> only fixes indent or style issues.
>
I've removed any changes to style in v2. Only makes things more
difficult to read.
> Also, I think the proper indentation is to use only one tab for the
> subsequent lines.
I've done this in v2.
>
>> @@ -598,6 +568,22 @@ rte_mempool_xmem_create(const char *name,
>> unsigned n, unsigned elt_size,
>> mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
>> mp->private_data_size = private_data_size;
>>
>> +/*
>> + * Since we have 4 combinations of the SP/SC/MP/MC, and stack,
>> + * examine the
>> + * flags to set the correct index into the handler table.
>> + */
>
> nit: comment style is not correct
>
Will fix.
>> +if (flags & MEMPOOL_F_USE_STACK)
>> +mp->handler_idx = rte_get_mempool_handler("stack");
>
> The stack handler does not exist yet, it is introduced in the next
> commit. I think this code should be moved in the next commit too.
Done in v2
>
>> @@ -622,6 +607,10 @@ rte_mempool_xmem_create(const char *name,
>> unsigned n, unsigned elt_size,
>>
>> mp->elt_va_end = mp->elt_va_start;
>>
>> +/* Parameters are setup. Call the mempool handler alloc */
>> +if ((rte_mempool_ext_alloc(mp, name, n, socket_id, flags)) == NULL)
>> +goto exit;
>> +
>
> I think some memory needs to be freed here. At least 'te'.
Done in v2
>> @@ -681,7 +670,9 @@ rte_mempool_dump_cache(FILE *f, const struct
>> rte_mempool *mp)
>> fprintf(f, "cache_size=%"PRIu32"\n", mp->cache_size);
>> for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>> cache_count = mp->local_cache[lcore_id].len;
>> -fprintf(f, "cache_count[%u]=%u\n", lcore_id, cache_count);
>> +if (cache_count > 0)
>> +fprintf(f, "cache_count[%u]=%u\n",
>> +lcore_id, cache_count);
>> count += cache_count;
>> }
>> fprintf(f, "total_cache_count=%u\n", count);
>
> This could also be moved in a separate commit.
Removed this change, as it's not really relevant to mempool manager
>> @@ -825,7 +815,7 @@ rte_mempool_dump(FILE *f, const struct
>> rte_mempool *mp)
>> mp->size);
>>
>> cache_count = rte_mempool_dump_cache(f, mp);
>> -common_count = rte_ring_count(mp->ring);
>> +common_count = /* rte_ring_count(mp->ring)*/0;
>> if ((cache_count + common_count) > mp->size)
>> common_count = mp->size - cache_count;
>> fprintf(f, " common_pool_count=%u\n", common_count);
>
> should it be rte_mempool_ext_get_count(mp) instead?
>
Done.
>
>
>> @@ -919,3 +909,111 @@ void rte_mempool_walk(void (*func)(const struct
>> rte_mempool *, void *),
>>
>> rte_rwlock_read_unlock(RTE_EAL_MEMPOOL_RWLOCK);
>> }
>> +
>> +
>> +/* create the mempool using and external mempool manager */
>> +struct rte_mempool *
>> +rte_mempool_create_ext(const char *name, unsigned n, unsigned elt_size,
>> +unsigned cache_size, unsigned private_data_size,
>> +rte_mempool_ctor_t *mp_init, void *mp_init_arg,
>> +rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
>>
[dpdk-dev] [PATCH 1/6] mempool: add external mempool manager support
On 2/19/2016 1:30 PM, Olivier MATZ wrote: > Hi David, > > On 02/16/2016 03:48 PM, David Hunt wrote: >> Adds the new rte_mempool_create_ext api and callback mechanism for >> external mempool handlers >> >> Modifies the existing rte_mempool_create to set up the handler_idx to >> the relevant mempool handler based on the handler name: >> ring_sp_sc >> ring_mp_mc >> ring_sp_mc >> ring_mp_sc >> >> v2: merges the duplicated code in rte_mempool_xmem_create and >> rte_mempool_create_ext into one common function. The old functions >> now call the new common function with the relevant parameters. >> >> Signed-off-by: David Hunt > I think the refactoring of rte_mempool_create() (adding of > mempool_create()) should go in another commit. It will make the > patches much easier to read. > > Also, I'm sorry but it seems that several comments or question I've made > in http://dpdk.org/ml/archives/dev/2016-February/032706.html are > not addressed. > > Examples: > - putting some part of the patch in separate commits > - meaning of "rt_pool" > - put_pool_bulk unclear comment > - should we also have get_pool_bulk stats? > - missing _MEMPOOL_STAT_ADD() in mempool_bulk() > - why internal in rte_mempool_internal.h? > - why default in rte_mempool_default.c? > - remaining references to stack handler (in a comment) > - ...? > > As you know, doing a proper code review takes a lot of time. If I > have to re-check all of my previous comments, it will take even > more. I'm not saying all my comments require a code change, but in case > you don't agree, please at least explain your opinion so we can debate > on the list. > Hi Olivier, Sincerest apologies. I had intended in coming back around to your original comments after refactoring the code. I will do that now. I did take them into consideration, but I see now that I need to do further work, such as a clearer name for rt_pool, etc. I will respond to your original email. Thanks David.
[dpdk-dev] [PATCH 2/6] mempool: add stack (lifo) based external mempool handler
On 2/19/2016 1:31 PM, Olivier MATZ wrote: > Hi David, > > On 02/16/2016 03:48 PM, David Hunt wrote: >> adds a simple stack based mempool handler >> >> Signed-off-by: David Hunt >> --- >> lib/librte_mempool/Makefile| 2 +- >> lib/librte_mempool/rte_mempool.c | 4 +- >> lib/librte_mempool/rte_mempool.h | 1 + >> lib/librte_mempool/rte_mempool_stack.c | 164 >> + >> 4 files changed, 169 insertions(+), 2 deletions(-) >> create mode 100644 lib/librte_mempool/rte_mempool_stack.c >> > I don't get what is the purpose of this handler. Is it an example > or is it something that could be useful for dpdk applications? > > If it's an example, we should find a way to put the code outside > the librte_mempool library, maybe in the test program. I see there > is also a "custom handler". Do we really need to have both? They are both example handlers. I agree that we could reduce down to one, and since the 'custom' handler has autotests, I would suggest we keep that one. The next question is where it should live. I agree that it's not ideal to have example code living in the same directory as the mempool library, but they are an integral part of the library itself. How about creating a handlers sub-directory? We could then keep all additional and sample handlers in there, away from the built-in handlers. Also, seeing as the handler code is intended to be part of the library, I think moving it out to the examples directory may confuse matters further. Regards, David.
[dpdk-dev] [PATCH 0/6] external mempool manager
On 2/19/2016 1:25 PM, Olivier MATZ wrote: > Hi, > > On 02/16/2016 03:48 PM, David Hunt wrote: >> Hi list. >> >> Here's the v2 version of a proposed patch for an external mempool manager > Just to notice the "v2" is missing in the title, it would help > to have it for next versions of the series. > Thanks, Olivier, I will ensure it's in the next patchset. Regards, David.
[dpdk-dev] [PATCH 2/2] examples: update to use new lpm lib for ip4
On 1/29/2016 12:12 PM, Michal Kobylinski wrote: > Update other applications to use new structures from LPM library for IPv4. > > Signed-off-by: Michal Kobylinski Acked-by: David Hunt
[dpdk-dev] [PATCH v2 1/2] lpm: extend ip4 next_hop and add config structure
On 1/29/2016 12:12 PM, Michal Kobylinski wrote: > As next_hop field for IPv4 is increased now the maximum number of tbl8s is > 2^24. > A new rte_lpm_config structure is used so LPM library will allocate > exactly the amount of memory which is necessary to hold application?s rules. > > Changed structures in LPM library: > rte_lpm_tbl24_entry and rte_lpm_tbl8_entry to one structure > rte_lpm_tbl_entry. > > Signed-off-by: Michal Kobylinski Acked-by: David Hunt
[dpdk-dev] [PATCH v3] mempool: reduce rte_mempool structure size
On 15/02/2016 10:15, Olivier MATZ wrote: > Hi David, > > On 02/15/2016 10:58 AM, Hunt, David wrote: >> On 12/02/2016 15:50, Olivier MATZ wrote: >>> - NEXT_ABI does make the code harder to read in this case, and I'm >>> thinking about the patchset from David Hunt (external mempool handler) >>> that will be in the same situation, and maybe also another patchset >>> I'm working on. >> >> Olivier, >> I'm working on that at the moment with the external mempool handler >> code. However, it crossed my mind that we have a choice to use symbol >> versioning OR use NEXT_ABI. Would one method be preferred over the other? > > I think symbol versioning should always be preferred when possible. > > In your case, as far as I remember, your are updating the rte_mempool > structure, which is accessed by static inline functions. I don't think > it is easily manageable with symbol versioning. Moreover, the ABI will > already be broken by Keith's patch, so I think it's less problematic > to have other patches breaking the ABI at the same time. OK, Thanks for that. I'll use NEXT_ABI in this case so. :) Regards, David.
[dpdk-dev] [PATCH v3] mempool: reduce rte_mempool structure size
On 12/02/2016 15:50, Olivier MATZ wrote: > - NEXT_ABI does make the code harder to read in this case, and I'm >thinking about the patchset from David Hunt (external mempool handler) >that will be in the same situation, and maybe also another patchset >I'm working on. Olivier, I'm working on that at the moment with the external mempool handler code. However, it crossed my mind that we have a choice to use symbol versioning OR use NEXT_ABI. Would one method be preferred over the other? Regards, David.
[dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support
On 04/02/2016 14:52, Olivier MATZ wrote:
> Hi David,
[snip]
Just a comment on one of your comments:
> Why not using a similar mechanism than what we have for PMDs?
>
> void rte_eal_driver_register(struct rte_driver *driver)
> {
> TAILQ_INSERT_TAIL(_driver_list, driver, next);
> }
>
> To do that, you just need to add a TAILQ_ENTRY() in your
> rte_mempool_handler structure. This would avoid to duplicate the
> structure into a static array whose size is limited.
>
> Accessing to the callbacks would be easier:
>
> return mp->mp_handler->put(mp->rt_pool, obj_table, n);
One of the iterations of the code did indeed use this mechanism, however
I ran into problems with multiple processes using the same mempool. In
that case, the 'mp_handler' element of the mempool in your return
statement is only valid for one of the processes. Hence the need for
and index that's valid for all processes rather than a pointer that's
valid for only one. And it's not easy to quickly index into an element
in a queue, hence the array of 16 mempool_handler structs.
[snip]
Rgds,
Dave.
[dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support
On 04/02/2016 14:52, Olivier MATZ wrote: > Hi David, > > Nice work, thanks ! > Please see some comments below. > > [snip] Olivier, Thanks for your comprehensive comments. I'm working on a v2 patch based on feedback already received from Jerin, and I'll be sure to include your feedback also. Many thanks, David.
[dpdk-dev] [PATCH 0/5] add external mempool manager
On 28/01/2016 17:26, Jerin Jacob wrote: > On Tue, Jan 26, 2016 at 05:25:50PM +, David Hunt wrote: >> Hi all on the list. >> >> Here's a proposed patch for an external mempool manager >> >> The External Mempool Manager is an extension to the mempool API that allows >> users to add and use an external mempool manager, which allows external >> memory >> subsystems such as external hardware memory management systems and software >> based memory allocators to be used with DPDK. > > I like this approach.It will be useful for external hardware memory > pool managers. > > BTW, Do you encounter any performance impact on changing to function > pointer based approach? Jerin, Thanks for your comments. The performance impacts I've seen depends on whether I'm using an object cache for the mempool or not. Without object cache, I see between 0-10% degradation. With object cache, I see a slight performance gain of between 0-5%. But that will most likely vary from system to system. >> The existing API to the internal DPDK mempool manager will remain unchanged >> and will be backward compatible. >> >> There are two aspects to external mempool manager. >>1. Adding the code for your new mempool handler. This is achieved by >> adding a >> new mempool handler source file into the librte_mempool library, and >> using the REGISTER_MEMPOOL_HANDLER macro. >>2. Using the new API to call rte_mempool_create_ext to create a new >> mempool >> using the name parameter to identify which handler to use. >> >> New API calls added >> 1. A new mempool 'create' function which accepts mempool handler name. >> 2. A new mempool 'rte_get_mempool_handler' function which accepts mempool >> handler name, and returns the index to the relevant set of callbacks for >> that mempool handler >> >> Several external mempool managers may be used in the same application. A new >> mempool can then be created by using the new 'create' function, providing the >> mempool handler name to point the mempool to the relevant mempool manager >> callback structure. >> >> The old 'create' function can still be called by legacy programs, and will >> internally work out the mempool handle based on the flags provided (single >> producer, single consumer, etc). By default handles are created internally to >> implement the built-in DPDK mempool manager and mempool types. >> >> The external mempool manager needs to provide the following functions. >> 1. alloc - allocates the mempool memory, and adds each object onto a >> ring >> 2. put - puts an object back into the mempool once an application has >> finished with it >> 3. get - gets an object from the mempool for use by the application >> 4. get_count - gets the number of available objects in the mempool >> 5. free - frees the mempool memory >> >> Every time a get/put/get_count is called from the application/PMD, the >> callback for that mempool is called. These functions are in the fastpath, >> and any unoptimised handlers may limit performance. >> >> The new APIs are as follows: >> >> 1. rte_mempool_create_ext >> >> struct rte_mempool * >> rte_mempool_create_ext(const char * name, unsigned n, >> unsigned cache_size, unsigned private_data_size, >> int socket_id, unsigned flags, >> const char * handler_name); >> >> 2. rte_get_mempool_handler >> >> int16_t >> rte_get_mempool_handler(const char *name); > > Do we need above public API as, in any case we need rte_mempool* pointer to > operate on mempools(which has the index anyway)? > > May a similar functional API with different name/return will be > better to figure out, given "name" registered or not in ethernet driver > which has dependency on a particular HW pool manager. Good point. An earlier revision required getting the index first, then passing that to the create_ext call. Now that the call is by name, the 'get' is mostly redundant. As you suggest, we may need an API for checking the existence of a particular manager/handler. Then again, we could always return an error from the create_ext api if it fails to find that handler. I'll remove the 'get' for the moment. Thanks, David.
[dpdk-dev] DPDK ArmV7 autotests
On 09/11/2015 17:46, Jan Viktorin wrote: > Here is the log. You an see, that many tests fail just because > of the missing hugetlb support. This is strange as I modified the > autotest_runner.py to inject "--no-huge --no-pci" options when it > detects armv7 architecture. I think publishing these test results to the mailing list confuses matters somewhat. The tests are not tuned for armv7, and there are many false negatives, giving the impression that the results are worse than they actually should be. The tests need to be analysed to see if they respect the --no-huge and --no-pci flags correctly, I suspect some tests are being run even though these flags are enabled. I believe enough of the tests pass to allow the initial version of the patch set to be accepted. We can look at the failing cases later, and improve the test suite as time goes on. Thanks, Dave.
[dpdk-dev] DPDK ArmV7 autotests
ed autotest: Fail [No prompt] [00m 00s] [04m > 45s] > Start group_7: Success [00m 00s] > PMD ring autotest: Fail [00m 00s] > Access list control autotest: Fail [Not found] [00m 00s] > Sched autotest:Success [00m 00s] [04m > 48s] > Start kni: Fail [No prompt] [00m 00s] > KNI autotest: Fail [No prompt] [00m 00s] [04m > 48s] > Start mempool_perf:Success [00m 00s] > Cycles autotest: Success [00m 01s] > Mempool performance autotest: Fail [Timeout][15m 00s] [19m > 50s] > Start memcpy_perf: Fail [No prompt] [00m 00s] > Memcpy performance autotest: Fail [No prompt] [00m 00s] [19m > 50s] > Start hash_perf: Fail [No prompt] [00m 00s] > Hash performance autotest: Fail [No prompt] [00m 00s] [19m > 51s] > Start power: Fail [No prompt] [00m 00s] > Power autotest:Fail [No prompt] [00m 00s] [19m > 51s] > Start power_acpi_cpufreq: Fail [No prompt] [00m 00s] > Power ACPI cpufreq autotest: Fail [No prompt] [00m 00s] [19m > 52s] > Start power_kvm_vm:Fail [No prompt] [00m 00s] > Power KVM VM autotest:Fail [No prompt] [00m 00s] [19m > 52s] > Start lpm6:Fail [No prompt] [00m 00s] > LPM6 autotest: Fail [No prompt] [00m 00s] [19m > 52s] > Start timer_perf: Fail [No prompt] [00m 00s] > Timer performance autotest:Fail [No prompt] [00m 00s] [19m > 53s] > Start ring_perf: Fail [No prompt] [00m 00s] > Ring performance autotest: Fail [No prompt] [00m 00s] [19m > 53s] > > Total run time: 19m 53s > Number of failed tests: 27 > = > > Regards > Jan > > On Mon, 9 Nov 2015 16:35:34 + > "Hunt, David" wrote: > >> Jan, >> I'm running some of the autotests, and I've got the following >> passes/fails. Have you run any of the failing tests on your platform? Do >> they pass? >> Dave. >> >> dpdk# make -C arm-armv7a-linuxapp-gcc fast_test >> Test name Test result Test Total >> = >> Timer autotest:Success [00m 42s] >> Debug autotest:Success [00m 00s] >> Errno autotest:Success [00m 00s] >> Meter autotest:Success [00m 00s] >> Common autotest: Success [00m 03s] >> Dump log history: Success [00m 00s] >> Dump rings:Success [00m 00s] >> Dump mempools: Success [00m 00s] [00m 50s] >> Memory autotest: Success [00m 00s] >> Read/write lock autotest: Success [00m 00s] >> Logs autotest: Success [00m 00s] >> CPU flags autotest:Success [00m 00s] >> Version autotest: Success [00m 00s] >> EAL filesystem autotest: Success [00m 00s] >> EAL flags autotest:Fail [Broken Test] >> Hash autotest: Fail [00m 00s] [00m 53s] >> LPM autotest: Fail [Not found] [00m 00s] >> IVSHMEM autotest: Fail [Not found] [00m 00s] >> Memcpy autotest: Success [00m 51s] >> Memzone autotest: Fail [00m 00s] >> String autotest: Success [00m 00s] >> Alarm autotest:Success [00m 16s] [02m 03s] >> PCI autotest: Fail [No PCI present] >> Malloc autotest: Success [00m 04s] >> Multi-process autotest:Fail [No PCI present] >> Mbuf autotest: Success [01m 00s] >> Per-lcore autotest:Success [00m 05s] >> Ring autotest: Success [00m 02s] [03m 21s] >> Spinlock autotest: Success [00m 15s] >> Byte order autotest: Success [00m 00s] >> TAILQ autotest:Success [00m
[dpdk-dev] [PATCH 15/15] armv8: config file update
On 05/11/2015 16:38, Jerin Jacob wrote: > disabled CONFIG_RTE_LIBRTE_FM10K_PMD and CONFIG_RTE_LIBRTE_I40E_PMD to fix > the compilation issues due to tmmintrin.h > > removed stale CONFIG_RTE_LIBRTE_EAL_HOTPLUG --snip-- Jerin, Each patch in a patch set should compile after its application so as not to break 'git disect'. Patch 15/15 fixes a compile issue introduced in patch 10, which breaks the compile, so I'd suggest rolling the patch 15 changes into patch 10 in the set. Dave
[dpdk-dev] [PATCH 00/12] DPDK armv8-a support
On 03/11/2015 16:38, Jerin Jacob wrote: > On Tue, Nov 03, 2015 at 02:17:38PM +0000, Hunt, David wrote: --snip-- >> and then it built fine, and I can run testpmd with my 82599's and run >> autotests. > > I ran autotest, "Mbuf autotest" stress failure is due strong vs weak ordering > issue. I will send the next version based on new patch being discussed > on ml. Jerin, I've marked my patch-set for the armv8 support as superseded in PatchWork. I'm happy for your patch-set to take precedence. If you're uploading another rev, I'll be sure to give it a test on my X-Gene board. Dave.
[dpdk-dev] [PATCH 00/12] DPDK armv8-a support
On 03/11/2015 13:09, Jerin Jacob wrote: > This is the v1 patchset for ARMv8 that now sits on top of the v6 patch > of the ARMv7 code by RehiveTech. It adds code into the same arm include > directory, reducing code duplication. > > Tested on an ThunderX arm 64-bit arm server board, with PCI slots. Passes > traffic > between two physical ports on an Intel 82599 dual-port 10Gig NIC. Should > work with many other NICS as long as long as there is no unaligned access to > device memory but not yet untested. I have your patchset building and running on an X-Gene based 8-core MP30AR0 system, passing traffic between two ports on and 82599 also. > Notes on arm64 kernel configuration: > >Tested on using Ubuntu 14.04 LTS with a 3.18 kernel and igb_uio. >ARM64 kernels does not have functional resource mapping of PCI memory >(PCI_MMAP), so the pci driver needs to be patched to enable this. The >symptom of this is when /sys/bus/pci/devices/:0X:00.Y directory is >missing the resource0...N files for mmapping the device memory. > >Following patch fixes the PCI resource mapping issue om armv8. >Its not yet up streamed.We are in the process of up streaming it. > >http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358906.html Good to see that there's a patch on the way for this. That fix looks almost exactly the same as the hack I did to my kernel :) I had a couple of small issues when patching/building: 1. Three of the files had an extra blank line at the end. Maybe worth running checkpatch on the patches. 'git am' was complaining. 2. I had problems compiling two drivers because they were attempting to include tmmintrin.h: ...dpdk/drivers/net/fm10k/fm10k_rxtx_vec.c:41:23: fatal error: tmmintrin.h: No such file or directory ...dpdk/drivers/net/i40e/i40e_rxtx_vec.c:43:23: fatal error: tmmintrin.h: No such file or directory To avoid this, I added the following two lines into defconfig_arm64-armv8a-linuxapp-gcc CONFIG_RTE_LIBRTE_FM10K_PMD=n CONFIG_RTE_LIBRTE_I40E_PMD=n and then it built fine, and I can run testpmd with my 82599's and run autotests. Thanks for that. Dave.
[dpdk-dev] [PATCH v6 00/15] Support ARMv7 architecture
On 03/11/2015 04:49, Jerin Jacob wrote: > On Tue, Nov 03, 2015 at 12:47:13AM +0100, Jan Viktorin wrote: >> Hello DPDK community, >> >> ARMv7 again, changes: >> >> * removed unnecessary code in the #ifndef RTE_FORCE_INTRINSICS .. #endif >> (atomic, spinlock, byteorder) >> * more splitting of headers to have 32/64 bit variants (atomic, cpuflags) >> * fixed cpuflags AT_PLATFORM > > Thanks Jan, Dave . > I will rework on the arm64 support based on this version. Jerin, I've got an updated ARMv8 patchset almost ready based on Jan's latest patchset (v6). Will I post it to the list? I suspect that your patchset will be the preferred option for acceptance into DPDK, but there may be one or two snippets of code worth merging together for the final patchset. Rgds, Dave.
[dpdk-dev] [PATCH v6 14/15] mk: Introduce ARMv7 architecture
On 03/11/2015 10:27, Jan Viktorin wrote: > On Tue, 3 Nov 2015 10:16:23 + > "Hunt, David" wrote: > >> On 02/11/2015 23:47, Jan Viktorin wrote: >> --snip-- >>> diff --git a/doc/guides/rel_notes/release_2_2.rst >>> b/doc/guides/rel_notes/release_2_2.rst >>> index be6f827..43a3a3c 100644 >>> --- a/doc/guides/rel_notes/release_2_2.rst >>> +++ b/doc/guides/rel_notes/release_2_2.rst >>> @@ -23,6 +23,11 @@ New Features >>> >>>* **Added vhost-user multiple queue support.** >> >> Jan, >> There's a small issue here. To apply cleanly on the latest head, this >> line needs to be >>* **Added port hotplug support to xenvirt.** > > Yes, the original v6 patchset was rebased on 82fb70207 (as stated in > the cover letter). I've force pushed an update (as resubmitting all the > series just due to this trivial conflict seems to be meaningless to me): Agreed, just noting for the future :) Dave
[dpdk-dev] [PATCH v6 14/15] mk: Introduce ARMv7 architecture
On 02/11/2015 23:47, Jan Viktorin wrote: --snip-- > diff --git a/doc/guides/rel_notes/release_2_2.rst > b/doc/guides/rel_notes/release_2_2.rst > index be6f827..43a3a3c 100644 > --- a/doc/guides/rel_notes/release_2_2.rst > +++ b/doc/guides/rel_notes/release_2_2.rst > @@ -23,6 +23,11 @@ New Features > > * **Added vhost-user multiple queue support.** Jan, There's a small issue here. To apply cleanly on the latest head, this line needs to be * **Added port hotplug support to xenvirt.** Rgds, Dave.
[dpdk-dev] [PATCH v3 1/6] eal/arm: add 64-bit armv8 version of rte_memcpy.h
On 02/11/2015 15:36, Jan Viktorin wrote:
> On Mon, 2 Nov 2015 15:26:19 +
--snip--
> It was looking like we can share a lot of common code for both
> architectures. I didn't know how much different are the cpuflags.
CPU flags for ARMv8 are looking like this now. Quite different to the
ARMv7 ones.
static const struct feature_entry cpu_feature_table[] = {
FEAT_DEF(FP,0x0001, 0, REG_HWCAP, 0)
FEAT_DEF(ASIMD, 0x0001, 0, REG_HWCAP, 1)
FEAT_DEF(EVTSTRM, 0x0001, 0, REG_HWCAP, 2)
FEAT_DEF(AES, 0x0001, 0, REG_HWCAP, 3)
FEAT_DEF(PMULL, 0x0001, 0, REG_HWCAP, 4)
FEAT_DEF(SHA1, 0x0001, 0, REG_HWCAP, 5)
FEAT_DEF(SHA2, 0x0001, 0, REG_HWCAP, 6)
FEAT_DEF(CRC32, 0x0001, 0, REG_HWCAP, 7)
FEAT_DEF(AARCH32, 0x0001, 0, REG_PLATFORM, 0)
FEAT_DEF(AARCH64, 0x0001, 0, REG_PLATFORM, 1)
};
> IMHO, it'd be better to have two directories arm and arm64. I thought
> to refer from arm64 to arm where possible. But I don't know whether is
> this possible with the DPDK build system.
I think both methodologies have their pros and cons. However, I'd lean
towards the common directory with the "filename_32/64.h" scheme, as that
similar to the x86 methodology, and we don't need to tweak the include
paths to pull files from multiple directories.
Dave
[dpdk-dev] [PATCH v3 1/6] eal/arm: add 64-bit armv8 version of rte_memcpy.h
On 02/11/2015 12:57, Jerin Jacob wrote: > On Mon, Nov 02, 2015 at 12:22:47PM +0000, Hunt, David wrote: >> Jerin, >> I've just benchmarked the libc version against the hand-coded version of >> the memcpy routines, and the libc wins in most cases. This code was just an >> initial attempt at optimising the memccpy's, so I feel that with the current >> benchmark results, it would better just to remove the assembly versions, and >> use the libc version for the initial release on ARMv8. >> Then, in the future, the ARMv8 experts are free to submit an optimised >> version as a patch in the future. Does that sound reasonable to you? > > Make sense. Based on my understanding, other blocks are also not optimized > for arm64. > So better to revert back to CONFIG_RTE_FORCE_INTRINSICS and > libc for initial version. > > BTW: I just tested ./arm64-armv8a-linuxapp-gcc/app/test and > "byteorder_autotest" is broken. I think existing arm64 code is not optimized > beyond CONFIG_RTE_FORCE_INTRINSICS. So better to use verified > CONFIG_RTE_FORCE_INTRINSICS scheme. Agreed. > if you guys are OK with arm and arm64 as two different platform then > I can summit the complete working patch for arm64.(as in my current source > code "arm64" is a different > platform(lib/librte_eal/common/include/arch/arm64/) Sure. That would be great. We initially started with two ARMv7 patch-sets, and Jan merged into one. Something similar could happen for the ARMv8 patch set. We just want to end up with the best implementation possible. :) Dave.
