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 8FFF8214A for ; Tue, 31 May 2016 11:09:49 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 31 May 2016 02:09:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,395,1459839600"; d="scan'208";a="711471736" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.49]) ([10.237.220.49]) by FMSMGA003.fm.intel.com with ESMTP; 31 May 2016 02:09:44 -0700 To: Jan Viktorin References: <1463665501-18325-2-git-send-email-david.hunt@intel.com> <20160523143511.7d30699b@pcviktorin.fit.vutbr.cz> Cc: dev@dpdk.org, olivier.matz@6wind.com, yuanhan.liu@linux.intel.com, pmatilai@redhat.com From: "Hunt, David" Message-ID: <574D54D6.1080409@intel.com> Date: Tue, 31 May 2016 10:09:42 +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: <20160523143511.7d30699b@pcviktorin.fit.vutbr.cz> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [dpdk-dev,v5,1/3] 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: Tue, 31 May 2016 09:09:50 -0000 Hi Jan, On 5/23/2016 1:35 PM, Jan Viktorin wrote: >> Until now, the objects stored in mempool mempool were internally stored a > s/mempool mempool/mempool/ > > stored _in_ a ring? Fixed. > > @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg) > n_get_bulk); > if (unlikely(ret < 0)) { > rte_mempool_dump(stdout, mp); > - rte_ring_dump(stdout, mp->ring); > /* in this case, objects are lost... */ > return -1; > } > I think, this should be in a separate patch explaining the reason to remove it. Done. Moved. >> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile >> index 43423e0..f19366e 100644 >> --- a/lib/librte_mempool/Makefile >> +++ b/lib/librte_mempool/Makefile >> @@ -42,6 +42,8 @@ LIBABIVER := 2 >> >> # all source are stored in SRCS-y >> SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c >> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c >> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_default.c >> # install includes >> SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h >> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c >> index 1ab6701..6ec2b3f 100644 >> --- a/lib/librte_mempool/rte_mempool.c >> +++ b/lib/librte_mempool/rte_mempool.c >> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj, phys_addr_t physaddr) >> #endif >> >> /* enqueue in ring */ >> - rte_ring_sp_enqueue(mp->ring, obj); >> + rte_mempool_ext_put_bulk(mp, &obj, 1); > I suppose this is OK, however, replacing "enqueue" by "put" (semantically) sounds to me > like a bug. Enqueue is put into a queue. Put is to drop a reference. Yes, Makes sense. Changed 'put' and 'get' to 'enqueue' and 'dequeue' > >> >> -/* create the internal ring */ >> -static int >> -rte_mempool_ring_create(struct rte_mempool *mp) >> -{ >> - int rg_flags = 0, ret; >> - char rg_name[RTE_RING_NAMESIZE]; >> - struct rte_ring *r; >> - >> - ret = snprintf(rg_name, sizeof(rg_name), >> - RTE_MEMPOOL_MZ_FORMAT, mp->name); >> - if (ret < 0 || ret >= (int)sizeof(rg_name)) >> - return -ENAMETOOLONG; >> - >> - /* ring flags */ >> - if (mp->flags & MEMPOOL_F_SP_PUT) >> - rg_flags |= RING_F_SP_ENQ; >> - if (mp->flags & MEMPOOL_F_SC_GET) >> - rg_flags |= RING_F_SC_DEQ; >> - >> - /* Allocate the ring that will be used to store objects. >> - * Ring functions will return appropriate errors if we are >> - * running as a secondary process etc., so no checks made >> - * in this function for that condition. >> - */ >> - r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1), >> - mp->socket_id, rg_flags); >> - if (r == NULL) >> - return -rte_errno; >> - >> - mp->ring = r; >> - mp->flags |= MEMPOOL_F_RING_CREATED; >> - return 0; >> -} > This is a big change. I suggest (if possible) to make a separate patch with > something like "replace rte_mempool_ring_create by ...". Where is this code > placed now? The code is not gone away, it's now part of the default handler, which uses a ring. It's in rte_mempool_default.c >> - >> /* free a memchunk allocated with rte_memzone_reserve() */ >> static void >> rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr, >> @@ -351,7 +317,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp) >> void *elt; >> >> while (!STAILQ_EMPTY(&mp->elt_list)) { >> - rte_ring_sc_dequeue(mp->ring, &elt); >> + rte_mempool_ext_get_bulk(mp, &elt, 1); > Similar as for put_bulk... Replacing "dequeue" by "get" (semantically) sounds to me > like a bug. Dequeue is drop from a queue. Get is to obtain a reference. Done >> (void)elt; >> STAILQ_REMOVE_HEAD(&mp->elt_list, next); >> mp->populated_size--; >> @@ -380,15 +346,18 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, >> unsigned i = 0; >> size_t off; >> struct rte_mempool_memhdr *memhdr; >> - int ret; >> >> /* create the internal ring if not already done */ >> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) { >> - ret = rte_mempool_ring_create(mp); >> - if (ret < 0) >> - return ret; >> + rte_errno = 0; >> + mp->pool = rte_mempool_ext_alloc(mp); >> + if (mp->pool == NULL) { >> + if (rte_errno == 0) >> + return -EINVAL; >> + else >> + return -rte_errno; >> + } >> } >> - > Is this a whitespace change? Accidental. Reverted >> + >> + /* >> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to >> + * set the correct index into the handler table. >> + */ >> + if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) >> + rte_mempool_set_handler(mp, "ring_sp_sc"); >> + else if (flags & MEMPOOL_F_SP_PUT) >> + rte_mempool_set_handler(mp, "ring_sp_mc"); >> + else if (flags & MEMPOOL_F_SC_GET) >> + rte_mempool_set_handler(mp, "ring_mp_sc"); >> + else >> + rte_mempool_set_handler(mp, "ring_mp_mc"); >> + > Do I understand it well that this code preserves behaviour of the previous API? > Because otherwise it looks strange. Yes. it's just to keep backward compatibility. It will also move to somewhere more sensible in the latest patch, rte_mempool_create rather than rte_mempool_create_empty. >> struct rte_mempool { >> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */ >> - struct rte_ring *ring; /**< Ring to store objects. */ >> + void *pool; /**< Ring or ext-pool to store objects. */ >> + /** >> + * Index into the array of structs containing callback fn 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. Any function pointers stored in the mempool >> + * directly would not be valid for secondary processes. >> + */ > I think, this comment should go to the rte_mempool_handler_table definition > leaving a here a short note about it. I've added a comment to rte_mempool_handler_table, and tweaked this comment somewhat. >> + int32_t handler_idx; >> const struct rte_memzone *mz; /**< Memzone where pool is allocated */ >> int flags; /**< Flags of the mempool. */ >> int socket_id; /**< Socket id passed at mempool creation. */ >> @@ -325,6 +334,175 @@ 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. */ >> + >> +/** Allocate the external pool. */ Note: for the next few comments you switched to commenting above the code , I've moved the comments to below the code, and replied. >> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp); >> + >> +/** Free the external pool. */ > What is the purpose of this callback? > What exactly does it allocate? > Some rte_mempool internals? > Or the memory? > What does it return? This is the main allocate function of the handler. It is up to the mempool handlers control. The handler's alloc function does whatever it needs to do to grab memory for this handler, and places a pointer to it in the *pool opaque pointer in the rte_mempool struct. In the default handler, *pool points to a ring, in other handlers, it will mostlikely point to a different type of data structure. It will be transparent to the application programmer. >> +typedef void (*rte_mempool_free_t)(void *p); >> + >> +/** Put an object in the external pool. */ > Why this *_free callback does not accept the rte_mempool param? > We're freeing the pool opaque data here. >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n); > What is the *p pointer? > What is the obj_table? > Why is it void *? > Why is it const? > The *p pointer is the opaque data for a given mempool handler (ring, array, linked list, etc) > Probably, "unsigned int n" is better. > >> + >> +/** Get an object from the external pool. */ >> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n); > Probably, "unsigned int n" is better. Done. >> + >> +/** Return the number of available objects in the external pool. */ > What is the purpose of the *_get_count callback? I guess it can introduce > race conditions... I think it depends on the implementation of the handlers get_count function, it must ensure not to return values greater than the size of the mempool. >> +typedef unsigned (*rte_mempool_get_count)(void *p); > unsigned int Sure. >> + >> +/** Structure defining a mempool handler. */ > Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops. > I believe that it explains the purpose of this struct better. It would improve > consistency in function names (the *_ext_* mark is very strange and inconsistent). I agree. I've gone through all the code and renamed to rte_mempool_handler_ops. >> +struct rte_mempool_handler { >> + char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */ >> + rte_mempool_alloc_t alloc; /**< Allocate the external pool. */ >> + 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 number of registered handlers */ >> + >> +/** Structure storing the table of registered handlers. */ >> +struct rte_mempool_handler_table { >> + rte_spinlock_t sl; /**< Spinlock for add/delete. */ >> + uint32_t num_handlers; /**< Number of handlers in the table. */ >> + /** Storage for all possible handlers. */ >> + struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX]; >> +}; > The handlers are implemented as an array due to multi-process access. > Is it correct? I'd expect a note about it here. Yes, you are correct. I've improved the comments. >> + >> +/** Array of registered handlers */ >> +extern struct rte_mempool_handler_table rte_mempool_handler_table; >> + >> +/** >> + * @internal Get the mempool handler from its index. >> + * >> + * @param handler_idx >> + * The index of the handler in the handler table. It must be a valid >> + * index: (0 <= idx < num_handlers). >> + * @return >> + * The pointer to the handler in the table. >> + */ >> +static struct rte_mempool_handler * >> +rte_mempool_handler_get(int handler_idx) >> +{ >> + return &rte_mempool_handler_table.handler[handler_idx]; > Is it always safe? Can we belive the handler_idx is inside the boundaries? > At least some RTE_VERIFY would be nice here... Agreed. Added: RTE_VERIFY(handler_idx < RTE_MEMPOOL_MAX_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_ext_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. > Should this doc be more specific about the possible failures? This is up to the handler. We cannot know what codes will be returned at this stage. >> + */ >> +static inline int >> +rte_mempool_ext_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n) >> +{ >> + struct rte_mempool_handler *handler; >> + >> + 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. > Should this doc be more specific about the possible failures? This is up to the handler. We cannot know what codes will be returned at this stage. >> + */ >> +static inline int >> +rte_mempool_ext_put_bulk(struct rte_mempool *mp, void * const *obj_table, >> + unsigned n) >> +{ >> + struct rte_mempool_handler *handler; >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + return handler->put(mp->pool, obj_table, n); >> +} >> + >> +/** >> + * @internal wrapper for external mempool manager get_count callback. >> + * >> + * @param mp >> + * Pointer to the memory pool. >> + * @return >> + * The number of available objects in the external pool. >> + */ >> +unsigned > unsigned int Done. >> +rte_mempool_ext_get_count(const struct rte_mempool *mp); >> + >> +/** >> + * @internal wrapper for external mempool manager free callback. >> + * >> + * @param mp >> + * Pointer to the memory pool. >> + */ >> +void >> +rte_mempool_ext_free(struct rte_mempool *mp); >> + >> +/** >> + * Set the handler of a mempool >> + * >> + * This can only be done on a mempool that is not populated, i.e. just after >> + * a call to rte_mempool_create_empty(). >> + * >> + * @param mp >> + * Pointer to the memory pool. >> + * @param name >> + * Name of the handler. >> + * @return >> + * - 0: Sucess; the new handler is configured. >> + * - <0: Error (errno) > Should this doc be more specific about the possible failures? > > The body of rte_mempool_set_handler does not set errno at all. > It returns e.g. -EEXIST. This is up to the handler. We cannot know what codes will be returned at this stage. errno was a cut-and paste error, this is now fixed. >> + */ >> +int >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name); >> + >> +/** >> + * Register an external pool handler. >> + * >> + * @param h >> + * Pointer to the external pool handler >> + * @return >> + * - >=0: Sucess; return the index of the handler in the table. >> + * - <0: Error (errno) > Should this doc be more specific about the possible failures? This is up to the handler. We cannot know what codes will be returned at this stage. errno was a cut-and paste error, this is now fixed. >> + */ >> +int rte_mempool_handler_register(struct rte_mempool_handler *h); >> + >> +/** >> + * Macro to statically register an external pool handler. >> + */ >> +#define MEMPOOL_REGISTER_HANDLER(h) \ >> + void mp_hdlr_init_##h(void); \ >> + void __attribute__((constructor, used)) mp_hdlr_init_##h(void) \ >> + { \ >> + rte_mempool_handler_register(&h); \ >> + } >> + > There might be a little catch. If there is no more room for handlers, calling the > rte_mempool_handler_register would fail silently as the error reporting does not > work when calling a constructor (or at least, this is my experience). > > Not a big deal but... I would hope that the developer would check this when adding new handlers. If there is no room for new handlers, then their new one will never work... >> >> 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"); >> - } >> -#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); >> -#endif >> + rte_mempool_ext_put_bulk(mp, obj_table, n); > This is a big change. Does it remove the RTE_LIBRTE_MEMPOOL_DEBUG config option > entirely? If so, I suggest to first do this in a separated patch and then > replace the original *_enqueue_bulk by your *_ext_put_bulk (or better *_ops_put_bulk > as I explain below). Well spotted. I have reverted this change. The mp_enqueue and sp_enqueue have been replaced with the handlers version, and I've added back in the DEBUG check with the rte_panic. >> >> + >> +static struct rte_mempool_handler handler_mp_mc = { >> + .name = "ring_mp_mc", >> + .alloc = common_ring_alloc, >> + .free = common_ring_free, >> + .put = common_ring_mp_put, >> + .get = common_ring_mc_get, >> + .get_count = common_ring_get_count, >> +}; >> + >> +static struct rte_mempool_handler handler_sp_sc = { >> + .name = "ring_sp_sc", >> + .alloc = common_ring_alloc, >> + .free = common_ring_free, >> + .put = common_ring_sp_put, >> + .get = common_ring_sc_get, >> + .get_count = common_ring_get_count, >> +}; >> + >> +static struct rte_mempool_handler handler_mp_sc = { >> + .name = "ring_mp_sc", >> + .alloc = common_ring_alloc, >> + .free = common_ring_free, >> + .put = common_ring_mp_put, >> + .get = common_ring_sc_get, >> + .get_count = common_ring_get_count, >> +}; >> + >> +static struct rte_mempool_handler handler_sp_mc = { >> + .name = "ring_sp_mc", >> + .alloc = common_ring_alloc, >> + .free = common_ring_free, >> + .put = common_ring_sp_put, >> + .get = common_ring_mc_get, >> + .get_count = common_ring_get_count, >> +}; >> + > Introducing those handlers can go as a separate patch. IMHO, that would simplify > the review process a lot. First introduce the mechanism, then add something > inside. > > I'd also note that those handlers are always available and what kind of memory > do they use... Done. Now added as a separate patch. They don't use any memory unless they are used. >> +#include >> +#include >> + >> +#include >> + >> +/* indirect jump table to support external memory pools */ >> +struct rte_mempool_handler_table rte_mempool_handler_table = { >> + .sl = RTE_SPINLOCK_INITIALIZER , >> + .num_handlers = 0 >> +}; >> + >> +/* add a new handler in rte_mempool_handler_table, return its index */ > It seems to me that there is no way how to put some opaque pointer into the > handler. In such case I would expect I can do something like: > > struct my_handler { > struct rte_mempool_handler h; > ... > } handler; > > rte_mempool_handler_register(&handler.h); > > But I cannot because you copy the contents of the handler. By the way, this > should be documented. > > How can I pass an opaque pointer here? The only way I see is through the > rte_mempool.pool. I think have addressed this in a later patch, in the discussions with Jerin on the list. But rather than passing data at register time, I pass it at create time (or rather set_handler). And a new config void has been added to the mempool struct for this purpose. > In that case, what about renaming the rte_mempool_handler > to rte_mempool_ops? Because semantically, it is not a handler, it just holds > the operations. > > This would improve some namings: > > rte_mempool_ext_alloc -> rte_mempool_ops_alloc > rte_mempool_ext_free -> rte_mempool_ops_free > rte_mempool_ext_get_count -> rte_mempool_ops_get_count > rte_mempool_handler_register -> rte_mempool_ops_register > > seems to be more readable to me. The *_ext_* mark does not say anything valuable. > It just scares a bit :). Agreed. Makes sense. The ext was intended to be 'external', but that's a bit too generic, and not very intuitive. the 'ops' tag seems better to me. I've change this in the latest patch. >> +/* wrapper to get available objects in an external pool handler */ >> +unsigned >> +rte_mempool_ext_get_count(const struct rte_mempool *mp) >> +{ >> + struct rte_mempool_handler *handler; >> + >> + handler = rte_mempool_handler_get(mp->handler_idx); >> + return handler->get_count(mp->pool); >> +} >> + >> +/* set the handler of a mempool */ > The doc comment should say "this sets a handler previously registered by > the rte_mempool_handler_register function ...". I was confused and didn't > understand how the handlers are inserted into the table. Done. --snip-- > Regards > Jan Thanks, Jan. Very comprehensive. I'll hopefully be pushing the latest patch to the list later today (Tuesday 31st) Regard, Dave.