From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Viktorin Subject: Re: [dpdk-dev,v5,1/3] mempool: support external handler Date: Tue, 31 May 2016 14:06:52 +0200 Message-ID: <20160531140652.018a03de@pcviktorin.fit.vutbr.cz> References: <1463665501-18325-2-git-send-email-david.hunt@intel.com> <20160523143511.7d30699b@pcviktorin.fit.vutbr.cz> <574D54D6.1080409@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, olivier.matz@6wind.com, yuanhan.liu@linux.intel.com, pmatilai@redhat.com To: "Hunt, David" Return-path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id E054E3239 for ; Tue, 31 May 2016 14:11:48 +0200 (CEST) In-Reply-To: <574D54D6.1080409@intel.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" On Tue, 31 May 2016 10:09:42 +0100 "Hunt, David" wrote: > Hi Jan, > [...] > > >> +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. Thanks for explanation. Please, add doc comments there. Should the *mp be const here? It is not clear to me whether the callback is expected to modify the mempool or not (eg. set the pool pointer). > > >> +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. Add doc comments... > > > >> +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) Again, doc comments... I don't like the obj_table representation to be an array of void *. I could see it already in DPDK for defining Ethernet driver queues, so, it's probably not an issue. I just say, I would prefer some basic type safety like struct rte_mempool_obj { void *p; }; Is there somebody with different opinions? [...] > >> + > >> +/** 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. Ok. I meant rte_mempool_ops because I find the word "handler" to be redundant. > [...] > >> +/** > >> + * 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. I don't think so. The rte_mempool_set_handler is not handler-specific: 116 /* set the handler of a mempool */ 117 int 118 rte_mempool_set_handler(struct rte_mempool *mp, const char *name) 119 { 120 struct rte_mempool_handler *handler = NULL; 121 unsigned i; 122 123 /* too late, the mempool is already populated */ 124 if (mp->flags & MEMPOOL_F_RING_CREATED) 125 return -EEXIST; Here, it returns -EEXIST. 126 127 for (i = 0; i < rte_mempool_handler_table.num_handlers; i++) { 128 if (!strcmp(name, rte_mempool_handler_table.handler[i].name)) { 129 handler = &rte_mempool_handler_table.handler[i]; 130 break; 131 } 132 } 133 134 if (handler == NULL) 135 return -EINVAL; And here, it returns -EINVAL. 136 137 mp->handler_idx = i; 138 return 0; 139 } So, it is possible to define those in the doc comment. > > > >> + */ > >> +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. Same, here... -ENOSPC, -EINVAL are returned in certain cases. And again, this call is not handler-specific. > [...] > >> > >> + > >> +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. Yes, that is what I've meant... What is the source of the allocations? Where does the common_ring_sc_get get memory from? Anyway, any documentation describing the goal of those four declarations would be helpful. > > > >> +#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. Ok, sounds promising. It just should be well specified to avoid situations when accessing the the pool_config before it is set. > > > 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. Again, note that I've suggested to avoid the word _handler_ entirely. [...] > > > Regards > > Jan > > Thanks, Jan. Very comprehensive. I'll hopefully be pushing the latest > patch to the list later today (Tuesday 31st) Cool, please CC me. Jan > > Regard, > Dave. > > -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic