[dpdk-dev,1/2] drivers/mempool: add stack mempool handler as driver

Message ID 1490004190-16892-1-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Shreyansh Jain March 20, 2017, 10:03 a.m. UTC
  CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
Stack mempool handler moved from lib/librte_mempool into drivers/mempool.

With this patch, the Stack mempool handler registration is optional and
toggled via the configuration file. In case disabled (N), it would imply
request for creating of mempool would fail.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 config/common_base                                 |   5 +
 drivers/Makefile                                   |   1 +
 drivers/mempool/Makefile                           |  36 +++++
 drivers/mempool/stack/Makefile                     |  55 ++++++++
 drivers/mempool/stack/rte_mempool_stack.c          | 147 +++++++++++++++++++++
 .../mempool/stack/rte_mempool_stack_version.map    |   4 +
 lib/librte_mempool/Makefile                        |   1 -
 lib/librte_mempool/rte_mempool_stack.c             | 147 ---------------------
 8 files changed, 248 insertions(+), 148 deletions(-)
 create mode 100644 drivers/mempool/Makefile
 create mode 100644 drivers/mempool/stack/Makefile
 create mode 100644 drivers/mempool/stack/rte_mempool_stack.c
 create mode 100644 drivers/mempool/stack/rte_mempool_stack_version.map
 delete mode 100644 lib/librte_mempool/rte_mempool_stack.c
  

Comments

Hunt, David March 20, 2017, 2:50 p.m. UTC | #1
On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>
> With this patch, the Stack mempool handler registration is optional and
> toggled via the configuration file. In case disabled (N), it would imply
> request for creating of mempool would fail.
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>   config/common_base                                 |   5 +
>   drivers/Makefile                                   |   1 +
>   drivers/mempool/Makefile                           |  36 +++++
>   drivers/mempool/stack/Makefile                     |  55 ++++++++
>   drivers/mempool/stack/rte_mempool_stack.c          | 147 +++++++++++++++++++++
>   .../mempool/stack/rte_mempool_stack_version.map    |   4 +
>   lib/librte_mempool/Makefile                        |   1 -
>   lib/librte_mempool/rte_mempool_stack.c             | 147 ---------------------
>   8 files changed, 248 insertions(+), 148 deletions(-)
>   create mode 100644 drivers/mempool/Makefile
>   create mode 100644 drivers/mempool/stack/Makefile
>   create mode 100644 drivers/mempool/stack/rte_mempool_stack.c
>   create mode 100644 drivers/mempool/stack/rte_mempool_stack_version.map
>   delete mode 100644 lib/librte_mempool/rte_mempool_stack.c
>
> diff --git a/config/common_base b/config/common_base
> index 37aa1e1..3e70aa0 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -464,6 +464,11 @@ CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE=512
>   CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n
>   
>   #
> +# Compile Mempool drivers
> +#
> +CONFIG_RTE_DRIVER_MEMPOOL_STACK=y
> +
> +#
>   # Compile librte_mbuf
>   #
>   CONFIG_RTE_LIBRTE_MBUF=y
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 81c03a8..193056b 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -31,6 +31,7 @@
>   
>   include $(RTE_SDK)/mk/rte.vars.mk
>   
> +DIRS-y += mempool
>   DIRS-y += net
>   DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto
>   
> diff --git a/drivers/mempool/Makefile b/drivers/mempool/Makefile
> new file mode 100644
> index 0000000..b256a34
> --- /dev/null
> +++ b/drivers/mempool/Makefile
> @@ -0,0 +1,36 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2017 NXP. 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 NXP 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 $(RTE_SDK)/mk/rte.vars.mk
> +
> +DIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += stack
> +
> +include $(RTE_SDK)/mk/rte.subdir.mk
> diff --git a/drivers/mempool/stack/Makefile b/drivers/mempool/stack/Makefile
> new file mode 100644
> index 0000000..f98eb5b
> --- /dev/null
> +++ b/drivers/mempool/stack/Makefile
> @@ -0,0 +1,55 @@
> +#   BSD LICENSE
> +#
> +#   Copyright(c) 2017 NXP.
> +#   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 NXP 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 $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_mempool_stack.a
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +
> +# Headers
> +CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
> +
> +EXPORT_MAP := rte_mempool_stack_version.map
> +
> +LIBABIVER := 1
> +
> +SRCS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += rte_mempool_stack.c
> +
> +# this lib depends upon:
> +DEPDIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += lib/librte_eal
> +DEPDIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += lib/librte_mempool
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/mempool/stack/rte_mempool_stack.c b/drivers/mempool/stack/rte_mempool_stack.c
> new file mode 100644
> index 0000000..817f77e
> --- /dev/null
> +++ b/drivers/mempool/stack/rte_mempool_stack.c
> @@ -0,0 +1,147 @@
> +/*-
> + *   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.
> + */
> +
> +#include <stdio.h>
> +#include <rte_mempool.h>
> +#include <rte_malloc.h>
> +
> +struct rte_mempool_stack {
> +	rte_spinlock_t sl;
> +
> +	uint32_t size;
> +	uint32_t len;
> +	void *objs[];
> +};
> +
> +static int
> +stack_alloc(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_stack *s;
> +	unsigned n = mp->size;
> +	int size = sizeof(*s) + (n+16)*sizeof(void *);
> +
> +	/* Allocate our local memory structure */
> +	s = rte_zmalloc_socket("mempool-stack",
> +			size,
> +			RTE_CACHE_LINE_SIZE,
> +			mp->socket_id);
> +	if (s == NULL) {
> +		RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
> +		return -ENOMEM;
> +	}
> +
> +	rte_spinlock_init(&s->sl);
> +
> +	s->size = n;
> +	mp->pool_data = s;
> +
> +	return 0;
> +}
> +
> +static int
> +stack_enqueue(struct rte_mempool *mp, void * const *obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_stack *s = mp->pool_data;
> +	void **cache_objs;
> +	unsigned index;
> +
> +	rte_spinlock_lock(&s->sl);
> +	cache_objs = &s->objs[s->len];
> +
> +	/* Is there sufficient space in the stack ? */
> +	if ((s->len + n) > s->size) {
> +		rte_spinlock_unlock(&s->sl);
> +		return -ENOBUFS;
> +	}
> +
> +	/* Add elements back into the cache */
> +	for (index = 0; index < n; ++index, obj_table++)
> +		cache_objs[index] = *obj_table;
> +
> +	s->len += n;
> +
> +	rte_spinlock_unlock(&s->sl);
> +	return 0;
> +}
> +
> +static int
> +stack_dequeue(struct rte_mempool *mp, void **obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_stack *s = mp->pool_data;
> +	void **cache_objs;
> +	unsigned index, len;
> +
> +	rte_spinlock_lock(&s->sl);
> +
> +	if (unlikely(n > s->len)) {
> +		rte_spinlock_unlock(&s->sl);
> +		return -ENOENT;
> +	}
> +
> +	cache_objs = s->objs;
> +
> +	for (index = 0, len = s->len - 1; index < n;
> +			++index, len--, obj_table++)
> +		*obj_table = cache_objs[len];
> +
> +	s->len -= n;
> +	rte_spinlock_unlock(&s->sl);
> +	return 0;
> +}
> +
> +static unsigned
> +stack_get_count(const struct rte_mempool *mp)
> +{
> +	struct rte_mempool_stack *s = mp->pool_data;
> +
> +	return s->len;
> +}
> +
> +static void
> +stack_free(struct rte_mempool *mp)
> +{
> +	rte_free((void *)(mp->pool_data));
> +}
> +
> +static struct rte_mempool_ops ops_stack = {
> +	.name = "stack",
> +	.alloc = stack_alloc,
> +	.free = stack_free,
> +	.enqueue = stack_enqueue,
> +	.dequeue = stack_dequeue,
> +	.get_count = stack_get_count
> +};
> +
> +MEMPOOL_REGISTER_OPS(ops_stack);
> diff --git a/drivers/mempool/stack/rte_mempool_stack_version.map b/drivers/mempool/stack/rte_mempool_stack_version.map
> new file mode 100644
> index 0000000..8591cc0
> --- /dev/null
> +++ b/drivers/mempool/stack/rte_mempool_stack_version.map
> @@ -0,0 +1,4 @@
> +DPDK_17.05 {
> +
> +	local: *;
> +};
> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
> index 057a6ab..a4c089e 100644
> --- a/lib/librte_mempool/Makefile
> +++ b/lib/librte_mempool/Makefile
> @@ -44,7 +44,6 @@ LIBABIVER := 2
>   SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
>   SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ops.c
>   SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ring.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
> deleted file mode 100644
> index 817f77e..0000000
> --- a/lib/librte_mempool/rte_mempool_stack.c
> +++ /dev/null
> @@ -1,147 +0,0 @@
> -/*-
> - *   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.
> - */
> -
> -#include <stdio.h>
> -#include <rte_mempool.h>
> -#include <rte_malloc.h>
> -
> -struct rte_mempool_stack {
> -	rte_spinlock_t sl;
> -
> -	uint32_t size;
> -	uint32_t len;
> -	void *objs[];
> -};
> -
> -static int
> -stack_alloc(struct rte_mempool *mp)
> -{
> -	struct rte_mempool_stack *s;
> -	unsigned n = mp->size;
> -	int size = sizeof(*s) + (n+16)*sizeof(void *);
> -
> -	/* Allocate our local memory structure */
> -	s = rte_zmalloc_socket("mempool-stack",
> -			size,
> -			RTE_CACHE_LINE_SIZE,
> -			mp->socket_id);
> -	if (s == NULL) {
> -		RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
> -		return -ENOMEM;
> -	}
> -
> -	rte_spinlock_init(&s->sl);
> -
> -	s->size = n;
> -	mp->pool_data = s;
> -
> -	return 0;
> -}
> -
> -static int
> -stack_enqueue(struct rte_mempool *mp, void * const *obj_table,
> -		unsigned n)
> -{
> -	struct rte_mempool_stack *s = mp->pool_data;
> -	void **cache_objs;
> -	unsigned index;
> -
> -	rte_spinlock_lock(&s->sl);
> -	cache_objs = &s->objs[s->len];
> -
> -	/* Is there sufficient space in the stack ? */
> -	if ((s->len + n) > s->size) {
> -		rte_spinlock_unlock(&s->sl);
> -		return -ENOBUFS;
> -	}
> -
> -	/* Add elements back into the cache */
> -	for (index = 0; index < n; ++index, obj_table++)
> -		cache_objs[index] = *obj_table;
> -
> -	s->len += n;
> -
> -	rte_spinlock_unlock(&s->sl);
> -	return 0;
> -}
> -
> -static int
> -stack_dequeue(struct rte_mempool *mp, void **obj_table,
> -		unsigned n)
> -{
> -	struct rte_mempool_stack *s = mp->pool_data;
> -	void **cache_objs;
> -	unsigned index, len;
> -
> -	rte_spinlock_lock(&s->sl);
> -
> -	if (unlikely(n > s->len)) {
> -		rte_spinlock_unlock(&s->sl);
> -		return -ENOENT;
> -	}
> -
> -	cache_objs = s->objs;
> -
> -	for (index = 0, len = s->len - 1; index < n;
> -			++index, len--, obj_table++)
> -		*obj_table = cache_objs[len];
> -
> -	s->len -= n;
> -	rte_spinlock_unlock(&s->sl);
> -	return 0;
> -}
> -
> -static unsigned
> -stack_get_count(const struct rte_mempool *mp)
> -{
> -	struct rte_mempool_stack *s = mp->pool_data;
> -
> -	return s->len;
> -}
> -
> -static void
> -stack_free(struct rte_mempool *mp)
> -{
> -	rte_free((void *)(mp->pool_data));
> -}
> -
> -static struct rte_mempool_ops ops_stack = {
> -	.name = "stack",
> -	.alloc = stack_alloc,
> -	.free = stack_free,
> -	.enqueue = stack_enqueue,
> -	.dequeue = stack_dequeue,
> -	.get_count = stack_get_count
> -};
> -
> -MEMPOOL_REGISTER_OPS(ops_stack);

Shreyansh,
     Could I suggest you add the parameter "--find-renames" when 
generating the patch files, as this will reduce the size of the patches 
significantly, making for easier review. The patch line count in this 
particular case would be reduced by approx 75%.
Regards,
Dave.
  
Shreyansh Jain March 21, 2017, 4:55 a.m. UTC | #2
Hello David,

On Monday 20 March 2017 08:20 PM, Hunt, David wrote:
>
> On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>>

<...>

>> -}
>> -
>> -static struct rte_mempool_ops ops_stack = {
>> -    .name = "stack",
>> -    .alloc = stack_alloc,
>> -    .free = stack_free,
>> -    .enqueue = stack_enqueue,
>> -    .dequeue = stack_dequeue,
>> -    .get_count = stack_get_count
>> -};
>> -
>> -MEMPOOL_REGISTER_OPS(ops_stack);
>
> Shreyansh,
>     Could I suggest you add the parameter "--find-renames" when
> generating the patch files, as this will reduce the size of the patches
> significantly, making for easier review. The patch line count in this
> particular case would be reduced by approx 75%.

Thanks for suggestion.
Yes, I forgot to use this option while creating this patch. If there
are comments and v2 needs to be created, I will keep this in mind.

> Regards,
> Dave.
>
>
  
Wiles, Keith March 21, 2017, 6:02 a.m. UTC | #3
> On Mar 20, 2017, at 11:55 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
> Hello David,
> 
> On Monday 20 March 2017 08:20 PM, Hunt, David wrote:
>> 
>> On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
>>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>>> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>>> 
> 
> <...>
> 
>>> -}
>>> -
>>> -static struct rte_mempool_ops ops_stack = {
>>> -    .name = "stack",
>>> -    .alloc = stack_alloc,
>>> -    .free = stack_free,
>>> -    .enqueue = stack_enqueue,
>>> -    .dequeue = stack_dequeue,
>>> -    .get_count = stack_get_count
>>> -};
>>> -
>>> -MEMPOOL_REGISTER_OPS(ops_stack);
>> 
>> Shreyansh,
>>    Could I suggest you add the parameter "--find-renames" when
>> generating the patch files, as this will reduce the size of the patches
>> significantly, making for easier review. The patch line count in this
>> particular case would be reduced by approx 75%.
> 
> Thanks for suggestion.
> Yes, I forgot to use this option while creating this patch. If there
> are comments and v2 needs to be created, I will keep this in mind.
> 
>> Regards,
>> Dave.

I guess I missed an email, but what is the advantage of moving the ring/stack files to the drivers directory as they are not drivers in the sense of a NIC PMD or any other driver. You can still enable/disable them in the config files right?

Regards,
Keith
  
Shreyansh Jain March 21, 2017, 6:25 a.m. UTC | #4
Hello Keith,

On Tuesday 21 March 2017 11:32 AM, Wiles, Keith wrote:
>
>> On Mar 20, 2017, at 11:55 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>>
>> Hello David,
>>
>> On Monday 20 March 2017 08:20 PM, Hunt, David wrote:
>>>
>>> On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
>>>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>>>> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>>>>
>>
>> <...>
>>
>>>> -}
>>>> -
>>>> -static struct rte_mempool_ops ops_stack = {
>>>> -    .name = "stack",
>>>> -    .alloc = stack_alloc,
>>>> -    .free = stack_free,
>>>> -    .enqueue = stack_enqueue,
>>>> -    .dequeue = stack_dequeue,
>>>> -    .get_count = stack_get_count
>>>> -};
>>>> -
>>>> -MEMPOOL_REGISTER_OPS(ops_stack);
>>>
>>> Shreyansh,
>>>    Could I suggest you add the parameter "--find-renames" when
>>> generating the patch files, as this will reduce the size of the patches
>>> significantly, making for easier review. The patch line count in this
>>> particular case would be reduced by approx 75%.
>>
>> Thanks for suggestion.
>> Yes, I forgot to use this option while creating this patch. If there
>> are comments and v2 needs to be created, I will keep this in mind.
>>
>>> Regards,
>>> Dave.
>
> I guess I missed an email, but what is the advantage of moving the ring/stack files to the drivers directory as they are not drivers in the sense of a NIC PMD or any other driver. You can still enable/disable them in the config files right?
>

Just as reference, following is where this was being discussed:

http://dpdk.org/ml/archives/dev/2017-March/059690.html
http://dpdk.org/ml/archives/dev/2017-March/059753.html
and
http://dpdk.org/ml/archives/dev/2017-March/060501.html

Also, a while back (I can't trace that mailing list exchange), it was
decided that all mempool drivers (stack, ring, others...) would be
moved to drivers/mempool.

For NXP's DPAA2 PMD, we use an offloaded mempool for which there was a
patchset by Hemant [1] which adds that driver to drivers/mempool. In
the same breadth, ring and stack are also being moved to
drivers/mempool as independent drivers (non-offloaded category).

[1] http://dpdk.org/ml/archives/dev/2017-March/060476.html

In my opinion, this would make the lib/* area free of handler/drivers
(almost) and it is a good change. Also, ring and stack use a 
'registration' mechanism - just like PMD and are good candidate to be 
treated as 'drivers' now even though not entirely like a PMD.

You see any downside of this?

> Regards,
> Keith
>
>

-
Shreyansh
  
Shreyansh Jain March 21, 2017, 6:28 a.m. UTC | #5
On Tuesday 21 March 2017 11:55 AM, Shreyansh Jain wrote:
> Hello Keith,
>
> On Tuesday 21 March 2017 11:32 AM, Wiles, Keith wrote:
>>
>>> On Mar 20, 2017, at 11:55 PM, Shreyansh Jain <shreyansh.jain@nxp.com>
>>> wrote:
>>>
>>> Hello David,
>>>
>>> On Monday 20 March 2017 08:20 PM, Hunt, David wrote:
>>>>
>>>> On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
>>>>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>>>>> Stack mempool handler moved from lib/librte_mempool into
>>>>> drivers/mempool.
>>>>>
>>>
>>> <...>
>>>
>>>>> -}
>>>>> -
>>>>> -static struct rte_mempool_ops ops_stack = {
>>>>> -    .name = "stack",
>>>>> -    .alloc = stack_alloc,
>>>>> -    .free = stack_free,
>>>>> -    .enqueue = stack_enqueue,
>>>>> -    .dequeue = stack_dequeue,
>>>>> -    .get_count = stack_get_count
>>>>> -};
>>>>> -
>>>>> -MEMPOOL_REGISTER_OPS(ops_stack);
>>>>
>>>> Shreyansh,
>>>>    Could I suggest you add the parameter "--find-renames" when
>>>> generating the patch files, as this will reduce the size of the patches
>>>> significantly, making for easier review. The patch line count in this
>>>> particular case would be reduced by approx 75%.
>>>
>>> Thanks for suggestion.
>>> Yes, I forgot to use this option while creating this patch. If there
>>> are comments and v2 needs to be created, I will keep this in mind.
>>>
>>>> Regards,
>>>> Dave.
>>
>> I guess I missed an email, but what is the advantage of moving the
>> ring/stack files to the drivers directory as they are not drivers in
>> the sense of a NIC PMD or any other driver. You can still
>> enable/disable them in the config files right?
>>
>
> Just as reference, following is where this was being discussed:
>
> http://dpdk.org/ml/archives/dev/2017-March/059690.html
> http://dpdk.org/ml/archives/dev/2017-March/059753.html
> and
> http://dpdk.org/ml/archives/dev/2017-March/060501.html
>
> Also, a while back (I can't trace that mailing list exchange), it was
> decided that all mempool drivers (stack, ring, others...) would be
> moved to drivers/mempool.

Just to add, discussion at that time was to have:

drivers/bus/<buses using rte_bus>
drivers/mempool/<Offloaded, or not-offloaded memory pool handlers>
drivers/net/<usual PMDs>

There was a drivers/common as well, but somehow it didn't go down well 
in discussions.

>
> For NXP's DPAA2 PMD, we use an offloaded mempool for which there was a
> patchset by Hemant [1] which adds that driver to drivers/mempool. In
> the same breadth, ring and stack are also being moved to
> drivers/mempool as independent drivers (non-offloaded category).
>
> [1] http://dpdk.org/ml/archives/dev/2017-March/060476.html
>
> In my opinion, this would make the lib/* area free of handler/drivers
> (almost) and it is a good change. Also, ring and stack use a
> 'registration' mechanism - just like PMD and are good candidate to be
> treated as 'drivers' now even though not entirely like a PMD.
>
> You see any downside of this?
>
>> Regards,
>> Keith
>>
>>
>
> -
> Shreyansh
>
>
  
Wiles, Keith March 21, 2017, 2:45 p.m. UTC | #6
> On Mar 21, 2017, at 1:25 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
> Hello Keith,
> 
> On Tuesday 21 March 2017 11:32 AM, Wiles, Keith wrote:
>> 
>>> On Mar 20, 2017, at 11:55 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>>> 
>>> Hello David,
>>> 
>>> On Monday 20 March 2017 08:20 PM, Hunt, David wrote:
>>>> 
>>>> On 20/3/2017 10:03 AM, Shreyansh Jain wrote:
>>>>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>>>>> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>>>>> 
>>> 
>>> <...>
>>> 
>>>>> -}
>>>>> -
>>>>> -static struct rte_mempool_ops ops_stack = {
>>>>> -    .name = "stack",
>>>>> -    .alloc = stack_alloc,
>>>>> -    .free = stack_free,
>>>>> -    .enqueue = stack_enqueue,
>>>>> -    .dequeue = stack_dequeue,
>>>>> -    .get_count = stack_get_count
>>>>> -};
>>>>> -
>>>>> -MEMPOOL_REGISTER_OPS(ops_stack);
>>>> 
>>>> Shreyansh,
>>>>   Could I suggest you add the parameter "--find-renames" when
>>>> generating the patch files, as this will reduce the size of the patches
>>>> significantly, making for easier review. The patch line count in this
>>>> particular case would be reduced by approx 75%.
>>> 
>>> Thanks for suggestion.
>>> Yes, I forgot to use this option while creating this patch. If there
>>> are comments and v2 needs to be created, I will keep this in mind.
>>> 
>>>> Regards,
>>>> Dave.
>> 
>> I guess I missed an email, but what is the advantage of moving the ring/stack files to the drivers directory as they are not drivers in the sense of a NIC PMD or any other driver. You can still enable/disable them in the config files right?
>> 
> 
> Just as reference, following is where this was being discussed:
> 
> http://dpdk.org/ml/archives/dev/2017-March/059690.html
> http://dpdk.org/ml/archives/dev/2017-March/059753.html
> and
> http://dpdk.org/ml/archives/dev/2017-March/060501.html
> 
> Also, a while back (I can't trace that mailing list exchange), it was
> decided that all mempool drivers (stack, ring, others...) would be
> moved to drivers/mempool.
> 
> For NXP's DPAA2 PMD, we use an offloaded mempool for which there was a
> patchset by Hemant [1] which adds that driver to drivers/mempool. In
> the same breadth, ring and stack are also being moved to
> drivers/mempool as independent drivers (non-offloaded category).
> 
> [1] http://dpdk.org/ml/archives/dev/2017-March/060476.html
> 
> In my opinion, this would make the lib/* area free of handler/drivers
> (almost) and it is a good change. Also, ring and stack use a 'registration' mechanism - just like PMD and are good candidate to be treated as 'drivers' now even though not entirely like a PMD.
> 
> You see any downside of this?

OK, I am up to date now. The only downside is splitting the mempool code up and making it a bit harder to manage, but it is a minor point. I do not see any stopper to prevent the change. Thanks.

> 
>> Regards,
>> Keith
>> 
>> 
> 
> -
> Shreyansh

Regards,
Keith
  
Olivier Matz March 24, 2017, 4:22 p.m. UTC | #7
Hi Shreyansh,

On Mon, 20 Mar 2017 15:33:09 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
> 
> With this patch, the Stack mempool handler registration is optional and
> toggled via the configuration file. In case disabled (N), it would imply
> request for creating of mempool would fail.
> 
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  config/common_base                                 |   5 +
>  drivers/Makefile                                   |   1 +
>  drivers/mempool/Makefile                           |  36 +++++
>  drivers/mempool/stack/Makefile                     |  55 ++++++++
>  drivers/mempool/stack/rte_mempool_stack.c          | 147 +++++++++++++++++++++
>  .../mempool/stack/rte_mempool_stack_version.map    |   4 +
>  lib/librte_mempool/Makefile                        |   1 -
>  lib/librte_mempool/rte_mempool_stack.c             | 147 ---------------------
>  8 files changed, 248 insertions(+), 148 deletions(-)
>  create mode 100644 drivers/mempool/Makefile
>  create mode 100644 drivers/mempool/stack/Makefile
>  create mode 100644 drivers/mempool/stack/rte_mempool_stack.c
>  create mode 100644 drivers/mempool/stack/rte_mempool_stack_version.map
>  delete mode 100644 lib/librte_mempool/rte_mempool_stack.c



I tried to pass the mempool autotest, and it issues a segfault.
I think the libraries are missing in rte.app.mk, so no handler is
registered.

Adding the following code in lib/librte_mempool/rte_mempool_ops.c
fixes the crash.

        ops = rte_mempool_get_ops(mp->ops_index);
+       if (ops == NULL || ops->alloc == NULL)
+               return -ENOTSUP;
        return ops->alloc(mp);

Now that drivers are not linked to the mempool library, it can
happen that there is no handler. Could you please add this patch in your
patchset?

I also checked that compilation works in shared lib mode. It looks good,
but I think there will be a behavior change if CONFIG_RTE_EAL_PMD_PATH
is empty (default). This option is probably used by distros to indicate
where dpdk plugins are installed, and when it is set, all drivers of
this directory are loaded, so in that case it won't change.

But when unset, the drivers have to be loaded manually, and with this
change, the mempool driver will have to be loaded with the -d eal option.
Could you please check what occurs in that case? At least see if it
displays a nice error or if it crashes. I suspect it will crash

Also, the MAINTAINERS file should be updated.


Thanks,
Olivier
  
Shreyansh Jain March 27, 2017, 4:54 a.m. UTC | #8
Hello Olivier,

On Friday 24 March 2017 09:52 PM, Olivier Matz wrote:
> Hi Shreyansh,
>
> On Mon, 20 Mar 2017 15:33:09 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> CONFIG_RTE_DRIVER_MEMPOOL_STACK option added to common_base.
>> Stack mempool handler moved from lib/librte_mempool into drivers/mempool.
>>
>> With this patch, the Stack mempool handler registration is optional and
>> toggled via the configuration file. In case disabled (N), it would imply
>> request for creating of mempool would fail.
>>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>>  config/common_base                                 |   5 +
>>  drivers/Makefile                                   |   1 +
>>  drivers/mempool/Makefile                           |  36 +++++
>>  drivers/mempool/stack/Makefile                     |  55 ++++++++
>>  drivers/mempool/stack/rte_mempool_stack.c          | 147 +++++++++++++++++++++
>>  .../mempool/stack/rte_mempool_stack_version.map    |   4 +
>>  lib/librte_mempool/Makefile                        |   1 -
>>  lib/librte_mempool/rte_mempool_stack.c             | 147 ---------------------
>>  8 files changed, 248 insertions(+), 148 deletions(-)
>>  create mode 100644 drivers/mempool/Makefile
>>  create mode 100644 drivers/mempool/stack/Makefile
>>  create mode 100644 drivers/mempool/stack/rte_mempool_stack.c
>>  create mode 100644 drivers/mempool/stack/rte_mempool_stack_version.map
>>  delete mode 100644 lib/librte_mempool/rte_mempool_stack.c
>
>
>
> I tried to pass the mempool autotest, and it issues a segfault.
> I think the libraries are missing in rte.app.mk, so no handler is
> registered.
>
> Adding the following code in lib/librte_mempool/rte_mempool_ops.c
> fixes the crash.
>
>         ops = rte_mempool_get_ops(mp->ops_index);
> +       if (ops == NULL || ops->alloc == NULL)
> +               return -ENOTSUP;
>         return ops->alloc(mp);
>
> Now that drivers are not linked to the mempool library, it can
> happen that there is no handler. Could you please add this patch in your
> patchset?

Indeed. I will add this code set as first patch. Thanks for suggestion/fix.

>
> I also checked that compilation works in shared lib mode. It looks good,
> but I think there will be a behavior change if CONFIG_RTE_EAL_PMD_PATH
> is empty (default). This option is probably used by distros to indicate
> where dpdk plugins are installed, and when it is set, all drivers of
> this directory are loaded, so in that case it won't change.
>
> But when unset, the drivers have to be loaded manually, and with this
> change, the mempool driver will have to be loaded with the -d eal option.
> Could you please check what occurs in that case? At least see if it
> displays a nice error or if it crashes. I suspect it will crash

Ok. I will try this and if there is any issue, fix it.

>
> Also, the MAINTAINERS file should be updated.

Yes, that is something I thought of updating but left it out before 
sending the patch.
One confirmation: I assume that maintainers need to be added with
"drivers/mempool/ring" and "drivers/mempool/stack" folders.
Who should be marked as maintainer - You (because of existing
lib/librte_mempool ownership) or I (because I have moved the code from
lib/librte_mempool)?

I think it would continue to be you but wanted to take your
confirmation before adding the lines.

>
>
> Thanks,
> Olivier
>
>

-
Shreyansh
  
Olivier Matz March 27, 2017, 7:22 a.m. UTC | #9
Hi Shreyansh,

[...]

> > Also, the MAINTAINERS file should be updated.  
> 
> Yes, that is something I thought of updating but left it out before 
> sending the patch.
> One confirmation: I assume that maintainers need to be added with
> "drivers/mempool/ring" and "drivers/mempool/stack" folders.
> Who should be marked as maintainer - You (because of existing
> lib/librte_mempool ownership) or I (because I have moved the code from
> lib/librte_mempool)?
> 
> I think it would continue to be you but wanted to take your
> confirmation before adding the lines.

Yes, I'll continue to maintain ring and stack handlers.

Thanks
Olivier
  
Shreyansh Jain March 28, 2017, 11:42 a.m. UTC | #10
Hello Olivier,

On Friday 24 March 2017 09:52 PM, Olivier Matz wrote:
[..]

> I tried to pass the mempool autotest, and it issues a segfault.
> I think the libraries are missing in rte.app.mk, so no handler is
> registered.

I have been trying to simulate the segfault that you are referring to 
above. But, I think it should not be the case. If a mempool handler is 
not registered (as librte_mempool_ring was not included in 
mk/rte.app.mk, so, no "ring_mp_mc"), the caller would get error.

The mempool_autotest is reporting:

--->8--
RTE>>mempool_autotest
cannot allocate mp_nocache mempool
Test Failed
--->8--

>
> Adding the following code in lib/librte_mempool/rte_mempool_ops.c
> fixes the crash.
>
>         ops = rte_mempool_get_ops(mp->ops_index);
> +       if (ops == NULL || ops->alloc == NULL)
> +               return -ENOTSUP;
>         return ops->alloc(mp);

Can you tell me for which case did your code reach 
rte_mempool_ops_alloc() and segfault?

In my case, librte_mempool_ring and librte_mempool_stack are not added 
to mk/rte.app.mk and it is static compilation.

>
> Now that drivers are not linked to the mempool library, it can
> happen that there is no handler. Could you please add this patch in your
> patchset?

Yes, once I can get this issue reproduced. Because I think there is one 
more place similar code should go (rte_mempool_ops_getcount).
As per what I can see, this would only happen if rte_mempool_xmem_create 
is called and then directly alloc is called. That is not happening for 
mempool_autotest.

-
Shreyansh
  
Olivier Matz March 29, 2017, 8:18 a.m. UTC | #11
On Tue, 28 Mar 2017 17:12:47 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> Hello Olivier,
> 
> On Friday 24 March 2017 09:52 PM, Olivier Matz wrote:
> [..]
> 
> > I tried to pass the mempool autotest, and it issues a segfault.
> > I think the libraries are missing in rte.app.mk, so no handler is
> > registered.  
> 
> I have been trying to simulate the segfault that you are referring to 
> above. But, I think it should not be the case. If a mempool handler is 
> not registered (as librte_mempool_ring was not included in 
> mk/rte.app.mk, so, no "ring_mp_mc"), the caller would get error.
> 
> The mempool_autotest is reporting:
> 
> --->8--  
> RTE>>mempool_autotest  
> cannot allocate mp_nocache mempool
> Test Failed
> --->8--  

Here are the reproduction steps:


git clone http://dpdk.org/git/dpdk
cd dpdk/
wget -O - http://dpdk.org/dev/patchwork/patch/21986/mbox | git am -
wget -O - http://dpdk.org/dev/patchwork/patch/21985/mbox | git am -
make config T=x86_64-native-linuxapp-gcc
make -j32 test-build
echo 128 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
mkdir -p /mnt/huge
mount -t hugetlbfs none /mnt/huge
echo mempool_autotest | ./build/app/test --
# segfault

# replay with debug
make -j32 test-build
make -j32 EXTRA_CFLAGS="-O0 -g" test-build
ulimit -c unlimited
echo mempool_autotest | ./build/app/test --
# segfault + core dump
gdb -c core ./build/app/test

(gdb) bt
#1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
    at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
#2  0x000000000064c1e7 in rte_mempool_populate_phys (mp=0x7f8816abdb40, 
    vaddr=0x7f880987a800 <error: Cannot access memory at address 0x7f880987a800>, 
    paddr=6958852096, len=26761152, free_cb=0x64c032 <rte_mempool_memchunk_mz_free>, 
    opaque=0x7f8822334d4c) at /root/dpdk/lib/librte_mempool/rte_mempool.c:359
#3  0x000000000064c9db in rte_mempool_populate_default (mp=0x7f8816abdb40)
    at /root/dpdk/lib/librte_mempool/rte_mempool.c:572
#4  0x000000000064d3d4 in rte_mempool_create (name=0x9b1ff0 "test_nocache", n=12671, 
    elt_size=2048, cache_size=0, private_data_size=0, mp_init=0x0, mp_init_arg=0x0, 
    obj_init=0x49f309 <my_obj_init>, obj_init_arg=0x0, socket_id=-1, flags=0)
    at /root/dpdk/lib/librte_mempool/rte_mempool.c:895
#5  0x00000000004a20ed in test_mempool () at /root/dpdk/test/test/test_mempool.c:519
#6  0x0000000000435189 in cmd_autotest_parsed (parsed_result=0x7ffe55006420, 
    cl=0x7c87090, data=0x0) at /root/dpdk/test/test/commands.c:103
#7  0x00000000006749df in cmdline_parse (cl=0x7c87090, 
    buf=0x7c870d8 "mempool_autotest\n")
    at /root/dpdk/lib/librte_cmdline/cmdline_parse.c:359
(gdb) up
#1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
    at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
101             return ops->alloc(mp);
(gdb) print ops
$1 = (struct rte_mempool_ops *) 0x4e69c00 <rte_mempool_ops_table+64>
(gdb) print *ops
$2 = {name = '\000' <repeats 31 times>, alloc = 0x0, free = 0x0, enqueue = 0x0, 
  dequeue = 0x0, get_count = 0x0}


Regards,
Olivier


> 
> >
> > Adding the following code in lib/librte_mempool/rte_mempool_ops.c
> > fixes the crash.
> >
> >         ops = rte_mempool_get_ops(mp->ops_index);
> > +       if (ops == NULL || ops->alloc == NULL)
> > +               return -ENOTSUP;
> >         return ops->alloc(mp);  
> 
> Can you tell me for which case did your code reach 
> rte_mempool_ops_alloc() and segfault?
> 
> In my case, librte_mempool_ring and librte_mempool_stack are not added 
> to mk/rte.app.mk and it is static compilation.
> 
> >
> > Now that drivers are not linked to the mempool library, it can
> > happen that there is no handler. Could you please add this patch in your
> > patchset?  
> 
> Yes, once I can get this issue reproduced. Because I think there is one 
> more place similar code should go (rte_mempool_ops_getcount).
> As per what I can see, this would only happen if rte_mempool_xmem_create 
> is called and then directly alloc is called. That is not happening for 
> mempool_autotest.
> 
> -
> Shreyansh
>
  
Shreyansh Jain March 29, 2017, 12:55 p.m. UTC | #12
Hello Olivier,

On Wednesday 29 March 2017 01:48 PM, Olivier Matz wrote:
> On Tue, 28 Mar 2017 17:12:47 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> Hello Olivier,
>>
>> On Friday 24 March 2017 09:52 PM, Olivier Matz wrote:
>> [..]
>>
>>> I tried to pass the mempool autotest, and it issues a segfault.
>>> I think the libraries are missing in rte.app.mk, so no handler is
>>> registered.
>>
>> I have been trying to simulate the segfault that you are referring to
>> above. But, I think it should not be the case. If a mempool handler is
>> not registered (as librte_mempool_ring was not included in
>> mk/rte.app.mk, so, no "ring_mp_mc"), the caller would get error.
>>
>> The mempool_autotest is reporting:
>>
>> --->8--
>> RTE>>mempool_autotest
>> cannot allocate mp_nocache mempool
>> Test Failed
>> --->8--
>
> Here are the reproduction steps:
>
>
> git clone http://dpdk.org/git/dpdk
> cd dpdk/
> wget -O - http://dpdk.org/dev/patchwork/patch/21986/mbox | git am -
> wget -O - http://dpdk.org/dev/patchwork/patch/21985/mbox | git am -
> make config T=x86_64-native-linuxapp-gcc
> make -j32 test-build
> echo 128 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
> mkdir -p /mnt/huge
> mount -t hugetlbfs none /mnt/huge
> echo mempool_autotest | ./build/app/test --
> # segfault

Thanks for the steps. I was able to reproduce this.
Don't know why it didn't work earlier.
one more comment below...

>
> # replay with debug
> make -j32 test-build
> make -j32 EXTRA_CFLAGS="-O0 -g" test-build
> ulimit -c unlimited
> echo mempool_autotest | ./build/app/test --
> # segfault + core dump
> gdb -c core ./build/app/test
>
> (gdb) bt
> #1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
>     at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
> #2  0x000000000064c1e7 in rte_mempool_populate_phys (mp=0x7f8816abdb40,
>     vaddr=0x7f880987a800 <error: Cannot access memory at address 0x7f880987a800>,
>     paddr=6958852096, len=26761152, free_cb=0x64c032 <rte_mempool_memchunk_mz_free>,
>     opaque=0x7f8822334d4c) at /root/dpdk/lib/librte_mempool/rte_mempool.c:359
> #3  0x000000000064c9db in rte_mempool_populate_default (mp=0x7f8816abdb40)
>     at /root/dpdk/lib/librte_mempool/rte_mempool.c:572

I think adding the code that you suggested is not the right place. The 
problem is not in rte_mempool_ops_alloc, where you had suggested the 
check for NULL.

The problem is in rte_mempool_create where return value for 
rte_mempool_set_ops_byname is not being checked.

When the libraries are not statically compiled in, 
rte_mempool_set_ops_byname is returning NULL, which rte_mempool_create 
doesn't handle and goes on to call rte_mempool_ops_alloc - eventually 
segfaulting.


> #4  0x000000000064d3d4 in rte_mempool_create (name=0x9b1ff0 "test_nocache", n=12671,
>     elt_size=2048, cache_size=0, private_data_size=0, mp_init=0x0, mp_init_arg=0x0,
>     obj_init=0x49f309 <my_obj_init>, obj_init_arg=0x0, socket_id=-1, flags=0)
>     at /root/dpdk/lib/librte_mempool/rte_mempool.c:895
> #5  0x00000000004a20ed in test_mempool () at /root/dpdk/test/test/test_mempool.c:519
> #6  0x0000000000435189 in cmd_autotest_parsed (parsed_result=0x7ffe55006420,
>     cl=0x7c87090, data=0x0) at /root/dpdk/test/test/commands.c:103
> #7  0x00000000006749df in cmdline_parse (cl=0x7c87090,
>     buf=0x7c870d8 "mempool_autotest\n")
>     at /root/dpdk/lib/librte_cmdline/cmdline_parse.c:359
> (gdb) up
> #1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
>     at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
> 101             return ops->alloc(mp);
> (gdb) print ops
> $1 = (struct rte_mempool_ops *) 0x4e69c00 <rte_mempool_ops_table+64>
> (gdb) print *ops
> $2 = {name = '\000' <repeats 31 times>, alloc = 0x0, free = 0x0, enqueue = 0x0,
>   dequeue = 0x0, get_count = 0x0}
>
>
> Regards,
> Olivier
>
>
>>
>>>
>>> Adding the following code in lib/librte_mempool/rte_mempool_ops.c
>>> fixes the crash.
>>>
>>>         ops = rte_mempool_get_ops(mp->ops_index);
>>> +       if (ops == NULL || ops->alloc == NULL)
>>> +               return -ENOTSUP;
>>>         return ops->alloc(mp);

If you think above explanation suffices, I will push a patch for error 
handling in rte_mempool_create returned by rte_mempool_set_ops_byname 
rather than above change originally suggested by you.

>>
>> Can you tell me for which case did your code reach
>> rte_mempool_ops_alloc() and segfault?
>>
>> In my case, librte_mempool_ring and librte_mempool_stack are not added
>> to mk/rte.app.mk and it is static compilation.
>>
>>>
>>> Now that drivers are not linked to the mempool library, it can
>>> happen that there is no handler. Could you please add this patch in your
>>> patchset?
>>
>> Yes, once I can get this issue reproduced. Because I think there is one
>> more place similar code should go (rte_mempool_ops_getcount).
>> As per what I can see, this would only happen if rte_mempool_xmem_create
>> is called and then directly alloc is called. That is not happening for
>> mempool_autotest.
>>
>> -
>> Shreyansh
>>
>
>
  
Olivier Matz March 30, 2017, 12:35 p.m. UTC | #13
On Wed, 29 Mar 2017 18:25:31 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> Hello Olivier,
> 
> On Wednesday 29 March 2017 01:48 PM, Olivier Matz wrote:
> > On Tue, 28 Mar 2017 17:12:47 +0530, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:  
> >> Hello Olivier,
> >>
> >> On Friday 24 March 2017 09:52 PM, Olivier Matz wrote:
> >> [..]
> >>  
> >>> I tried to pass the mempool autotest, and it issues a segfault.
> >>> I think the libraries are missing in rte.app.mk, so no handler is
> >>> registered.  
> >>
> >> I have been trying to simulate the segfault that you are referring to
> >> above. But, I think it should not be the case. If a mempool handler is
> >> not registered (as librte_mempool_ring was not included in
> >> mk/rte.app.mk, so, no "ring_mp_mc"), the caller would get error.
> >>
> >> The mempool_autotest is reporting:
> >>  
> >> --->8--  
> >> RTE>>mempool_autotest  
> >> cannot allocate mp_nocache mempool
> >> Test Failed  
> >> --->8--  
> >
> > Here are the reproduction steps:
> >
> >
> > git clone http://dpdk.org/git/dpdk
> > cd dpdk/
> > wget -O - http://dpdk.org/dev/patchwork/patch/21986/mbox | git am -
> > wget -O - http://dpdk.org/dev/patchwork/patch/21985/mbox | git am -
> > make config T=x86_64-native-linuxapp-gcc
> > make -j32 test-build
> > echo 128 > /sys/devices/system/node/node0/hugepages/hugepages-2048kB/nr_hugepages
> > mkdir -p /mnt/huge
> > mount -t hugetlbfs none /mnt/huge
> > echo mempool_autotest | ./build/app/test --
> > # segfault  
> 
> Thanks for the steps. I was able to reproduce this.
> Don't know why it didn't work earlier.
> one more comment below...
> 
> >
> > # replay with debug
> > make -j32 test-build
> > make -j32 EXTRA_CFLAGS="-O0 -g" test-build
> > ulimit -c unlimited
> > echo mempool_autotest | ./build/app/test --
> > # segfault + core dump
> > gdb -c core ./build/app/test
> >
> > (gdb) bt
> > #1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
> >     at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
> > #2  0x000000000064c1e7 in rte_mempool_populate_phys (mp=0x7f8816abdb40,
> >     vaddr=0x7f880987a800 <error: Cannot access memory at address 0x7f880987a800>,
> >     paddr=6958852096, len=26761152, free_cb=0x64c032 <rte_mempool_memchunk_mz_free>,
> >     opaque=0x7f8822334d4c) at /root/dpdk/lib/librte_mempool/rte_mempool.c:359
> > #3  0x000000000064c9db in rte_mempool_populate_default (mp=0x7f8816abdb40)
> >     at /root/dpdk/lib/librte_mempool/rte_mempool.c:572  
> 
> I think adding the code that you suggested is not the right place. The 
> problem is not in rte_mempool_ops_alloc, where you had suggested the 
> check for NULL.
> 
> The problem is in rte_mempool_create where return value for 
> rte_mempool_set_ops_byname is not being checked.
> 
> When the libraries are not statically compiled in, 
> rte_mempool_set_ops_byname is returning NULL, which rte_mempool_create 
> doesn't handle and goes on to call rte_mempool_ops_alloc - eventually 
> segfaulting.

Yes, you're right, it's better to check the return value of
rte_mempool_set_ops_byname() in rte_mempool_create().



> > #4  0x000000000064d3d4 in rte_mempool_create (name=0x9b1ff0 "test_nocache", n=12671,
> >     elt_size=2048, cache_size=0, private_data_size=0, mp_init=0x0, mp_init_arg=0x0,
> >     obj_init=0x49f309 <my_obj_init>, obj_init_arg=0x0, socket_id=-1, flags=0)
> >     at /root/dpdk/lib/librte_mempool/rte_mempool.c:895
> > #5  0x00000000004a20ed in test_mempool () at /root/dpdk/test/test/test_mempool.c:519
> > #6  0x0000000000435189 in cmd_autotest_parsed (parsed_result=0x7ffe55006420,
> >     cl=0x7c87090, data=0x0) at /root/dpdk/test/test/commands.c:103
> > #7  0x00000000006749df in cmdline_parse (cl=0x7c87090,
> >     buf=0x7c870d8 "mempool_autotest\n")
> >     at /root/dpdk/lib/librte_cmdline/cmdline_parse.c:359
> > (gdb) up
> > #1  0x000000000064dead in rte_mempool_ops_alloc (mp=0x7f8816abdb40)
> >     at /root/dpdk/lib/librte_mempool/rte_mempool_ops.c:101
> > 101             return ops->alloc(mp);
> > (gdb) print ops
> > $1 = (struct rte_mempool_ops *) 0x4e69c00 <rte_mempool_ops_table+64>
> > (gdb) print *ops
> > $2 = {name = '\000' <repeats 31 times>, alloc = 0x0, free = 0x0, enqueue = 0x0,
> >   dequeue = 0x0, get_count = 0x0}
> >
> >
> > Regards,
> > Olivier
> >
> >  
> >>  
> >>>
> >>> Adding the following code in lib/librte_mempool/rte_mempool_ops.c
> >>> fixes the crash.
> >>>
> >>>         ops = rte_mempool_get_ops(mp->ops_index);
> >>> +       if (ops == NULL || ops->alloc == NULL)
> >>> +               return -ENOTSUP;
> >>>         return ops->alloc(mp);  
> 
> If you think above explanation suffices, I will push a patch for error 
> handling in rte_mempool_create returned by rte_mempool_set_ops_byname 
> rather than above change originally suggested by you.

Yes, please. Thanks!

Olivier


> 
> >>
> >> Can you tell me for which case did your code reach
> >> rte_mempool_ops_alloc() and segfault?
> >>
> >> In my case, librte_mempool_ring and librte_mempool_stack are not added
> >> to mk/rte.app.mk and it is static compilation.
> >>  
> >>>
> >>> Now that drivers are not linked to the mempool library, it can
> >>> happen that there is no handler. Could you please add this patch in your
> >>> patchset?  
> >>
> >> Yes, once I can get this issue reproduced. Because I think there is one
> >> more place similar code should go (rte_mempool_ops_getcount).
> >> As per what I can see, this would only happen if rte_mempool_xmem_create
> >> is called and then directly alloc is called. That is not happening for
> >> mempool_autotest.
> >>
> >> -
> >> Shreyansh
> >>  
> >
> >  
>
  

Patch

diff --git a/config/common_base b/config/common_base
index 37aa1e1..3e70aa0 100644
--- a/config/common_base
+++ b/config/common_base
@@ -464,6 +464,11 @@  CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE=512
 CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n
 
 #
+# Compile Mempool drivers
+#
+CONFIG_RTE_DRIVER_MEMPOOL_STACK=y
+
+#
 # Compile librte_mbuf
 #
 CONFIG_RTE_LIBRTE_MBUF=y
diff --git a/drivers/Makefile b/drivers/Makefile
index 81c03a8..193056b 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -31,6 +31,7 @@ 
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
+DIRS-y += mempool
 DIRS-y += net
 DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += crypto
 
diff --git a/drivers/mempool/Makefile b/drivers/mempool/Makefile
new file mode 100644
index 0000000..b256a34
--- /dev/null
+++ b/drivers/mempool/Makefile
@@ -0,0 +1,36 @@ 
+#   BSD LICENSE
+#
+#   Copyright(c) 2017 NXP. 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 NXP 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 $(RTE_SDK)/mk/rte.vars.mk
+
+DIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += stack
+
+include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/drivers/mempool/stack/Makefile b/drivers/mempool/stack/Makefile
new file mode 100644
index 0000000..f98eb5b
--- /dev/null
+++ b/drivers/mempool/stack/Makefile
@@ -0,0 +1,55 @@ 
+#   BSD LICENSE
+#
+#   Copyright(c) 2017 NXP.
+#   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 NXP 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 $(RTE_SDK)/mk/rte.vars.mk
+
+#
+# library name
+#
+LIB = librte_mempool_stack.a
+
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+
+# Headers
+CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
+
+EXPORT_MAP := rte_mempool_stack_version.map
+
+LIBABIVER := 1
+
+SRCS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += rte_mempool_stack.c
+
+# this lib depends upon:
+DEPDIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += lib/librte_eal
+DEPDIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_STACK) += lib/librte_mempool
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/mempool/stack/rte_mempool_stack.c b/drivers/mempool/stack/rte_mempool_stack.c
new file mode 100644
index 0000000..817f77e
--- /dev/null
+++ b/drivers/mempool/stack/rte_mempool_stack.c
@@ -0,0 +1,147 @@ 
+/*-
+ *   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.
+ */
+
+#include <stdio.h>
+#include <rte_mempool.h>
+#include <rte_malloc.h>
+
+struct rte_mempool_stack {
+	rte_spinlock_t sl;
+
+	uint32_t size;
+	uint32_t len;
+	void *objs[];
+};
+
+static int
+stack_alloc(struct rte_mempool *mp)
+{
+	struct rte_mempool_stack *s;
+	unsigned n = mp->size;
+	int size = sizeof(*s) + (n+16)*sizeof(void *);
+
+	/* Allocate our local memory structure */
+	s = rte_zmalloc_socket("mempool-stack",
+			size,
+			RTE_CACHE_LINE_SIZE,
+			mp->socket_id);
+	if (s == NULL) {
+		RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
+		return -ENOMEM;
+	}
+
+	rte_spinlock_init(&s->sl);
+
+	s->size = n;
+	mp->pool_data = s;
+
+	return 0;
+}
+
+static int
+stack_enqueue(struct rte_mempool *mp, void * const *obj_table,
+		unsigned n)
+{
+	struct rte_mempool_stack *s = mp->pool_data;
+	void **cache_objs;
+	unsigned index;
+
+	rte_spinlock_lock(&s->sl);
+	cache_objs = &s->objs[s->len];
+
+	/* Is there sufficient space in the stack ? */
+	if ((s->len + n) > s->size) {
+		rte_spinlock_unlock(&s->sl);
+		return -ENOBUFS;
+	}
+
+	/* Add elements back into the cache */
+	for (index = 0; index < n; ++index, obj_table++)
+		cache_objs[index] = *obj_table;
+
+	s->len += n;
+
+	rte_spinlock_unlock(&s->sl);
+	return 0;
+}
+
+static int
+stack_dequeue(struct rte_mempool *mp, void **obj_table,
+		unsigned n)
+{
+	struct rte_mempool_stack *s = mp->pool_data;
+	void **cache_objs;
+	unsigned index, len;
+
+	rte_spinlock_lock(&s->sl);
+
+	if (unlikely(n > s->len)) {
+		rte_spinlock_unlock(&s->sl);
+		return -ENOENT;
+	}
+
+	cache_objs = s->objs;
+
+	for (index = 0, len = s->len - 1; index < n;
+			++index, len--, obj_table++)
+		*obj_table = cache_objs[len];
+
+	s->len -= n;
+	rte_spinlock_unlock(&s->sl);
+	return 0;
+}
+
+static unsigned
+stack_get_count(const struct rte_mempool *mp)
+{
+	struct rte_mempool_stack *s = mp->pool_data;
+
+	return s->len;
+}
+
+static void
+stack_free(struct rte_mempool *mp)
+{
+	rte_free((void *)(mp->pool_data));
+}
+
+static struct rte_mempool_ops ops_stack = {
+	.name = "stack",
+	.alloc = stack_alloc,
+	.free = stack_free,
+	.enqueue = stack_enqueue,
+	.dequeue = stack_dequeue,
+	.get_count = stack_get_count
+};
+
+MEMPOOL_REGISTER_OPS(ops_stack);
diff --git a/drivers/mempool/stack/rte_mempool_stack_version.map b/drivers/mempool/stack/rte_mempool_stack_version.map
new file mode 100644
index 0000000..8591cc0
--- /dev/null
+++ b/drivers/mempool/stack/rte_mempool_stack_version.map
@@ -0,0 +1,4 @@ 
+DPDK_17.05 {
+
+	local: *;
+};
diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
index 057a6ab..a4c089e 100644
--- a/lib/librte_mempool/Makefile
+++ b/lib/librte_mempool/Makefile
@@ -44,7 +44,6 @@  LIBABIVER := 2
 SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
 SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ops.c
 SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_ring.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
deleted file mode 100644
index 817f77e..0000000
--- a/lib/librte_mempool/rte_mempool_stack.c
+++ /dev/null
@@ -1,147 +0,0 @@ 
-/*-
- *   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.
- */
-
-#include <stdio.h>
-#include <rte_mempool.h>
-#include <rte_malloc.h>
-
-struct rte_mempool_stack {
-	rte_spinlock_t sl;
-
-	uint32_t size;
-	uint32_t len;
-	void *objs[];
-};
-
-static int
-stack_alloc(struct rte_mempool *mp)
-{
-	struct rte_mempool_stack *s;
-	unsigned n = mp->size;
-	int size = sizeof(*s) + (n+16)*sizeof(void *);
-
-	/* Allocate our local memory structure */
-	s = rte_zmalloc_socket("mempool-stack",
-			size,
-			RTE_CACHE_LINE_SIZE,
-			mp->socket_id);
-	if (s == NULL) {
-		RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
-		return -ENOMEM;
-	}
-
-	rte_spinlock_init(&s->sl);
-
-	s->size = n;
-	mp->pool_data = s;
-
-	return 0;
-}
-
-static int
-stack_enqueue(struct rte_mempool *mp, void * const *obj_table,
-		unsigned n)
-{
-	struct rte_mempool_stack *s = mp->pool_data;
-	void **cache_objs;
-	unsigned index;
-
-	rte_spinlock_lock(&s->sl);
-	cache_objs = &s->objs[s->len];
-
-	/* Is there sufficient space in the stack ? */
-	if ((s->len + n) > s->size) {
-		rte_spinlock_unlock(&s->sl);
-		return -ENOBUFS;
-	}
-
-	/* Add elements back into the cache */
-	for (index = 0; index < n; ++index, obj_table++)
-		cache_objs[index] = *obj_table;
-
-	s->len += n;
-
-	rte_spinlock_unlock(&s->sl);
-	return 0;
-}
-
-static int
-stack_dequeue(struct rte_mempool *mp, void **obj_table,
-		unsigned n)
-{
-	struct rte_mempool_stack *s = mp->pool_data;
-	void **cache_objs;
-	unsigned index, len;
-
-	rte_spinlock_lock(&s->sl);
-
-	if (unlikely(n > s->len)) {
-		rte_spinlock_unlock(&s->sl);
-		return -ENOENT;
-	}
-
-	cache_objs = s->objs;
-
-	for (index = 0, len = s->len - 1; index < n;
-			++index, len--, obj_table++)
-		*obj_table = cache_objs[len];
-
-	s->len -= n;
-	rte_spinlock_unlock(&s->sl);
-	return 0;
-}
-
-static unsigned
-stack_get_count(const struct rte_mempool *mp)
-{
-	struct rte_mempool_stack *s = mp->pool_data;
-
-	return s->len;
-}
-
-static void
-stack_free(struct rte_mempool *mp)
-{
-	rte_free((void *)(mp->pool_data));
-}
-
-static struct rte_mempool_ops ops_stack = {
-	.name = "stack",
-	.alloc = stack_alloc,
-	.free = stack_free,
-	.enqueue = stack_enqueue,
-	.dequeue = stack_dequeue,
-	.get_count = stack_get_count
-};
-
-MEMPOOL_REGISTER_OPS(ops_stack);