From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 661322C4E for ; Thu, 2 Jun 2016 13:23:44 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP; 02 Jun 2016 04:23:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,405,1459839600"; d="scan'208";a="979394602" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.85]) ([10.237.221.85]) by fmsmga001.fm.intel.com with ESMTP; 02 Jun 2016 04:23:42 -0700 To: Jan Viktorin References: <1463665501-18325-1-git-send-email-david.hunt@intel.com> <1464797998-76690-1-git-send-email-david.hunt@intel.com> <1464797998-76690-2-git-send-email-david.hunt@intel.com> <20160601195456.47ac2a7c@pcviktorin.fit.vutbr.cz> Cc: dev@dpdk.org, olivier.matz@6wind.com, jerin.jacob@caviumnetworks.com From: "Hunt, David" Message-ID: <5750173D.5080404@intel.com> Date: Thu, 2 Jun 2016 12:23:41 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160601195456.47ac2a7c@pcviktorin.fit.vutbr.cz> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 1/5] mempool: support external handler 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: Thu, 02 Jun 2016 11:23:45 -0000 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 array instead. You're probably right, but it's always been called obj_table, and I'm not sure this patchset is the place to change it. Maybe a follow up patch? >> + */ >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n); > unsigned int Done > >> + >> +/** Get an object from the external pool. */ >> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n); > unsigned int Done > >> + >> +/** Return the number of available objects in the external pool. */ > Is the number of available objects or the total number of all objects > (so probably a constant value)? It's intended to be the umber of available objects > >> +typedef unsigned (*rte_mempool_get_count)(void *p); > Should it be const void *p? Yes, it could be. Changed. > >> + >> +/** Structure defining a mempool handler. */ > What about: > > /** Structure defining mempool operations. */ Changed. >> +struct rte_mempool_ops { >> + char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */ > s/RTE_MEMPOOL_HANDLER_NAMESIZE/RTE_MEMPOOL_OPS_NAMESIZE/ Done >> + rte_mempool_alloc_t alloc; /**< Allocate the external pool. */ > What about: > > Allocate the private data for this rte_mempool_ops. Changed to /**< 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 the number of available objs. */ >> +} __rte_cache_aligned; >> + >> +#define RTE_MEMPOOL_MAX_HANDLER_IDX 16 /**< Max registered handlers */ > s/RTE_MEMPOOL_MAX_HANDLER_IDX/RTE_MEMPOOL_MAX_OPS_IDX/ Changed >> + >> +/** >> + * Structure storing the table of registered handlers, each of which contain >> + * the function pointers for the mempool handler functions. >> + * Each process has it's own storage for this handler struct aray so that >> + * the mempools can be shared across primary and secondary processes. >> + * The indices used to access the array are valid across processes, whereas >> + * any function pointers stored directly in the mempool struct would not be. >> + * This results in us simply having "handler_idx" in the mempool struct. >> + */ >> +struct rte_mempool_handler_table { > s/rte_mempool_handler_table/rte_mempool_ops_table/ Done >> + rte_spinlock_t sl; /**< Spinlock for add/delete. */ >> + uint32_t num_handlers; /**< Number of handlers in the table. */ > s/num_handlers/num_ops/ Done >> + /** >> + * Storage for all possible handlers. >> + */ >> + struct rte_mempool_ops handler_ops[RTE_MEMPOOL_MAX_HANDLER_IDX]; > s/handler_ops/ops/ Done >> +} __rte_cache_aligned; >> + >> +/** Array of registered handlers */ >> +extern struct rte_mempool_handler_table rte_mempool_handler_table; > s/rte_mempool_handler_table/rte_mempool_ops_table/ Done >> + >> +/** >> + * @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 inline struct rte_mempool_ops * >> +rte_mempool_handler_get(int handler_idx) > rte_mempool_ops_get(int ops_idx) Done >> +{ >> + return &rte_mempool_handler_table.handler_ops[handler_idx]; >> +} >> + >> +/** >> + * @internal wrapper for external mempool manager alloc callback. >> + * >> + * @param mp >> + * Pointer to the memory pool. >> + * @return >> + * The opaque pointer to the external pool. >> + */ >> +void * >> +rte_mempool_ops_alloc(struct rte_mempool *mp); >> + >> +/** >> + * @internal wrapper for external mempool manager get 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 handler get function. >> + */ >> +static inline int >> +rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp, >> + void **obj_table, unsigned n) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + return handler->get(mp->pool, obj_table, n); >> +} >> + >> +/** >> + * @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 handler put function. >> + */ >> +static inline int >> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table, >> + unsigned n) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + * @return >> + * - 0: Sucess; the new handler is configured. >> + * - EINVAL - Invalid handler name provided >> + * - EEXIST - mempool already has a handler assigned > They are returned as -EINVAL and -EEXIST. > > IMHO, using "-" here is counter-intuitive: > > - EINVAL > > does it mean a positive with "-" or negative value? EINVAL is positive, so it's returning negative. Common usage in DPDK, afaics. >> + */ >> +int >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name); > rte_mempool_set_ops > > What about rte_mempool_set_ops_byname? Not a big deal... I agree. rte_mempool_set_ops_byname >> + >> +/** >> + * Register an external pool handler. > Register mempool operations Done >> + * >> + * @param h >> + * Pointer to the external pool handler >> + * @return >> + * - >=0: Sucess; return the index of the handler in the table. >> + * - EINVAL - missing callbacks while registering handler >> + * - ENOSPC - the maximum number of handlers has been reached > - -EINVAL > - -ENOSPC :) >> + */ >> +int rte_mempool_handler_register(const struct rte_mempool_ops *h); > rte_mempool_ops_register Done >> + >> +/** >> + * Macro to statically register an external pool handler. > What about adding: > > Note that the rte_mempool_ops_register fails silently here when > more then RTE_MEMPOOL_MAX_OPS_IDX is registered. Done >> + */ >> +#define MEMPOOL_REGISTER_HANDLER(h) > MEMPOOL_REGISTER_OPS Done > \ >> + void mp_hdlr_init_##h(void); \ >> + void __attribute__((constructor, used)) mp_hdlr_init_##h(void) \ >> + { \ >> + rte_mempool_handler_register(&h); \ >> + } >> + >> /** >> * An object callback function for mempool. >> * >> @@ -774,7 +980,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, >> cache->len += n; >> >> if (cache->len >= flushthresh) { >> - rte_ring_mp_enqueue_bulk(mp->ring, &cache->objs[cache_size], >> + rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache_size], >> cache->len - cache_size); >> cache->len = cache_size; >> } >> @@ -785,19 +991,10 @@ ring_enqueue: >> >> /* push remaining objects in ring */ >> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG >> - if (is_mp) { >> - if (rte_ring_mp_enqueue_bulk(mp->ring, obj_table, n) < 0) >> - rte_panic("cannot put objects in mempool\n"); >> - } >> - else { >> - if (rte_ring_sp_enqueue_bulk(mp->ring, obj_table, n) < 0) >> - rte_panic("cannot put objects in mempool\n"); >> - } >> + if (rte_mempool_ops_enqueue_bulk(mp, obj_table, n) < 0) >> + rte_panic("cannot put objects in mempool\n"); >> #else >> - if (is_mp) >> - rte_ring_mp_enqueue_bulk(mp->ring, obj_table, n); >> - else >> - rte_ring_sp_enqueue_bulk(mp->ring, obj_table, n); >> + rte_mempool_ops_enqueue_bulk(mp, obj_table, n); >> #endif >> } >> >> @@ -922,7 +1119,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) >> */ >> static inline int __attribute__((always_inline)) >> __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, >> - unsigned n, int is_mc) >> + unsigned n, __rte_unused int is_mc) > unsigned int Done >> { >> int ret; >> struct rte_mempool_cache *cache; >> @@ -945,7 +1142,8 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, >> uint32_t req = n + (cache_size - cache->len); >> >> /* How many do we require i.e. number to fill the cache + the request */ >> - ret = rte_ring_mc_dequeue_bulk(mp->ring, &cache->objs[cache->len], req); >> + ret = rte_mempool_ops_dequeue_bulk(mp, >> + &cache->objs[cache->len], req); >> if (unlikely(ret < 0)) { >> /* >> * In the offchance that we are buffer constrained, >> @@ -972,10 +1170,7 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, >> ring_dequeue: >> >> /* get remaining objects from ring */ >> - if (is_mc) >> - ret = rte_ring_mc_dequeue_bulk(mp->ring, obj_table, n); >> - else >> - ret = rte_ring_sc_dequeue_bulk(mp->ring, obj_table, n); >> + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); >> >> if (ret < 0) >> __MEMPOOL_STAT_ADD(mp, get_fail, n); >> diff --git a/lib/librte_mempool/rte_mempool_handler.c b/lib/librte_mempool/rte_mempool_handler.c >> new file mode 100644 >> index 0000000..ed85d65 >> --- /dev/null >> +++ b/lib/librte_mempool/rte_mempool_handler.c > rte_mempool_ops.c Done. > >> @@ -0,0 +1,141 @@ >> +/*- >> + * BSD LICENSE >> + * >> + * Copyright(c) 2016 Intel Corporation. All rights reserved. >> + * Copyright(c) 2016 6WIND S.A. >> + * 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 >> +#include >> + >> +#include >> + >> +/* indirect jump table to support external memory pools */ >> +struct rte_mempool_handler_table rte_mempool_handler_table = { > rte_mempool_ops_table Done >> + .sl = RTE_SPINLOCK_INITIALIZER , >> + .num_handlers = 0 > num_ops Done >> +}; >> + >> +/* add a new handler in rte_mempool_handler_table, return its index */ >> +int >> +rte_mempool_handler_register(const struct rte_mempool_ops *h) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + int16_t handler_idx; > ops_idx Done >> + >> + rte_spinlock_lock(&rte_mempool_handler_table.sl); >> + >> + if (rte_mempool_handler_table.num_handlers >= >> + RTE_MEMPOOL_MAX_HANDLER_IDX) { >> + rte_spinlock_unlock(&rte_mempool_handler_table.sl); >> + RTE_LOG(ERR, MEMPOOL, >> + "Maximum number of mempool handlers exceeded\n"); >> + return -ENOSPC; >> + } >> + >> + if (h->put == NULL || h->get == NULL || h->get_count == NULL) { >> + rte_spinlock_unlock(&rte_mempool_handler_table.sl); >> + RTE_LOG(ERR, MEMPOOL, >> + "Missing callback while registering mempool handler\n"); >> + return -EINVAL; >> + } >> + >> + handler_idx = rte_mempool_handler_table.num_handlers++; >> + handler = &rte_mempool_handler_table.handler_ops[handler_idx]; >> + snprintf(handler->name, sizeof(handler->name), "%s", h->name); >> + handler->alloc = h->alloc; >> + handler->put = h->put; >> + handler->get = h->get; >> + handler->get_count = h->get_count; >> + >> + rte_spinlock_unlock(&rte_mempool_handler_table.sl); >> + >> + return handler_idx; >> +} >> + >> +/* wrapper to allocate an external pool handler */ >> +void * >> +rte_mempool_ops_alloc(struct rte_mempool *mp) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + if (handler->alloc == NULL) >> + return NULL; >> + return handler->alloc(mp); >> +} >> + >> +/* wrapper to free an external pool handler */ >> +void >> +rte_mempool_ops_free(struct rte_mempool *mp) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + if (handler->free == NULL) >> + return; >> + return handler->free(mp); >> +} >> + >> +/* wrapper to get available objects in an external pool handler */ >> +unsigned int >> +rte_mempool_ops_get_count(const struct rte_mempool *mp) >> +{ >> + struct rte_mempool_ops *handler; > *ops Done >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + return handler->get_count(mp->pool); >> +} >> + >> +/* sets a handler previously registered by rte_mempool_handler_register */ >> +int >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name) >> +{ >> + struct rte_mempool_ops *handler = NULL; > *ops Done >> + unsigned i; >> + >> + /* too late, the mempool is already populated */ >> + if (mp->flags & MEMPOOL_F_RING_CREATED) >> + return -EEXIST; >> + >> + for (i = 0; i < rte_mempool_handler_table.num_handlers; i++) { >> + if (!strcmp(name, >> + rte_mempool_handler_table.handler_ops[i].name)) { >> + handler = &rte_mempool_handler_table.handler_ops[i]; >> + break; >> + } >> + } >> + >> + if (handler == NULL) >> + return -EINVAL; >> + >> + mp->handler_idx = i; >> + return 0; >> +} > Regards > Jan Thanks, Dave.