From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 6BEF329D6 for ; Fri, 4 Mar 2016 10:05:28 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1ablhJ-0008Pn-Ri; Fri, 04 Mar 2016 10:06:58 +0100 To: "Hunt, David" , dev@dpdk.org References: <1453829155-1366-1-git-send-email-david.hunt@intel.com> <1453829155-1366-2-git-send-email-david.hunt@intel.com> <56B365A0.3080206@6wind.com> <56D599FE.90706@intel.com> From: Olivier MATZ X-Enigmail-Draft-Status: N1110 Message-ID: <56D94FD1.2050901@6wind.com> Date: Fri, 4 Mar 2016 10:05:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <56D599FE.90706@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Mar 2016 09:05:28 -0000 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 >>> 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? >>> /** >>> * 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. >>> @@ -769,8 +853,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void * >>> const *obj_table, >>> >>> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 >>> /* cache is not enabled or single producer or non-EAL thread */ >>> - if (unlikely(cache_size == 0 || is_mp == 0 || >>> - lcore_id >= RTE_MAX_LCORE)) >>> + if (unlikely(cache_size == 0 || lcore_id >= RTE_MAX_LCORE)) >>> goto ring_enqueue; >>> >>> /* Go straight to ring if put would overflow mem allocated for >>> cache */ >> >> If I understand well, we now always use the cache, even if the mempool >> is single-producer. I was wondering if it would have a performance >> impact... I suppose that using the cache is more efficient than the ring >> in single-producer mode, so it may increase performance. Do you have an >> idea of the impact here? > > I've seen very little in performance gain, maybe a couple of percent for > some tests, and up to 10% drop for some single core tests. I'll do some > more specific testing based on SP versus MP. OK thanks! >>> diff --git a/lib/librte_mempool/rte_mempool_default.c >>> b/lib/librte_mempool/rte_mempool_default.c >>> new file mode 100644 >>> index 0000000..2493dc1 >>> --- /dev/null >>> +++ b/lib/librte_mempool/rte_mempool_default.c >>> +#include "rte_mempool_internal.h" >>> + >>> +/* >>> + * Indirect jump table to support external memory pools >>> + */ >>> +struct rte_mempool_handler_list mempool_handler_list = { >>> + .sl = RTE_SPINLOCK_INITIALIZER , >>> + .num_handlers = 0 >>> +}; >>> + >>> +/* TODO Convert to older mechanism of an array of stucts */ >>> +int16_t >>> +add_handler(struct rte_mempool_handler *h) >>> +{ >>> + int16_t handler_idx; >>> + >>> + /* */ >>> + rte_spinlock_lock(&mempool_handler_list.sl); >>> + >>> + /* Check whether jump table has space */ >>> + if (mempool_handler_list.num_handlers >= >>> RTE_MEMPOOL_MAX_HANDLER_IDX) { >>> + rte_spinlock_unlock(&mempool_handler_list.sl); >>> + RTE_LOG(ERR, MEMPOOL, >>> + "Maximum number of mempool handlers exceeded\n"); >>> + return -1; >>> + } >>> + >>> + if ((h->put == NULL) || (h->get == NULL) || >>> + (h->get_count == NULL)) { >>> + rte_spinlock_unlock(&mempool_handler_list.sl); >>> + RTE_LOG(ERR, MEMPOOL, >>> + "Missing callback while registering mempool >>> handler\n"); >>> + return -1; >>> + } >>> + >>> + /* 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(&mempool_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(&dev_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); >> >> instead of: >> >> return (mempool_handler_list.handler[mp->handler_idx].put) >> (mp->rt_pool, obj_table, n); >> >> If we really want to copy the handlers somewhere, it could be in >> the mempool structure. It would avoid an extra dereference >> (note the first '.' instead of '->'): >> >> return mp.mp_handler->put(mp->rt_pool, obj_table, n); >> >> After doing that, we could ask ourself if the wrappers are still >> useful or not. I would have say that they could be removed. >> >> >> The spinlock could be kept, although it may look a bit overkill: >> - I don't expect to have several loading at the same time >> - There is no unregister() function, so there is no risk to >> browse the list atomically >> > > 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. >> 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 think they should go in rte_mempool_default.c They could either go in rte_mempool.c or in another file rte_mempool_handler.c. >>> --- /dev/null >>> +++ b/lib/librte_mempool/rte_mempool_internal.h >> >> Is it the proper name? >> We could imagine a mempool handler provided by a plugin, and >> in this case this code should go in rte_mempool.h. > > I was trying to keep the public APIs in rte_mempool.h, and aal the > private stuff in rte_mempool_internal.h. Maybe a better name would be > rte_mempool_private.h? Are these functions internal? I mean, is it possible for an application or an external PMD (.so) to provide its own handler? I think it would be really interesting to have this capability. Then, I would prefer to have this either in rte_mempool.h or in rte_mempool_handler.h (that would be coherent with the .c) >>> + >>> +struct rte_mempool_handler { >>> + char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool handler */ >> >> I would use a const char * here instead. >> > > Would we then have to allocate the memory for the string elsewhere? I > would have thought this is the more straightforward method. My initial question was, can we just have something like this: // my_handler.c static struct rte_mempool_handler handler_stack = { .name = "my_handler", .alloc = my_alloc, ... }; // rte_mempool_handler.c int16_t rte_mempool_register_handler(struct rte_mempool_handler *h) { ... handler->name = h->name; /* instead of snprintf */ ... } But it won't be possible as the structures will be shared with a secondary process. So the name[RTE_MEMPOOL_NAMESIZE] is fine. Regards, Olivier