From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hunt, David" Subject: Re: [PATCH v2 1/3] mempool: add stack (lifo) mempool handler Date: Wed, 15 Jun 2016 11:10:11 +0100 Message-ID: <57612983.7030709@intel.com> References: <1462472982-49782-1-git-send-email-david.hunt@intel.com> <1463669335-30378-1-git-send-email-david.hunt@intel.com> <1463669335-30378-2-git-send-email-david.hunt@intel.com> <5742FDA6.5070108@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: Olivier Matz , dev@dpdk.org Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 6C7FBC4E2 for ; Wed, 15 Jun 2016 12:10:13 +0200 (CEST) In-Reply-To: <5742FDA6.5070108@6wind.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 0000000..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(&s->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(&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 -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 default handler, it would be valuable to have them > in the documentation. > I could certainly add this to the docs, and mention the recommendation to use local cache. Regards, Dave.