From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 7B0322E7B for ; Tue, 31 May 2016 15:47:25 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 31 May 2016 06:47:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,395,1459839600"; d="scan'208";a="965804859" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.49]) ([10.237.220.49]) by orsmga001.jf.intel.com with ESMTP; 31 May 2016 06:47:22 -0700 To: Jan Viktorin References: <1463665501-18325-2-git-send-email-david.hunt@intel.com> <20160523143511.7d30699b@pcviktorin.fit.vutbr.cz> <574D54D6.1080409@intel.com> <20160531140652.018a03de@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: <574D95E9.4020504@intel.com> Date: Tue, 31 May 2016 14:47:21 +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: <20160531140652.018a03de@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 13:47:26 -0000 On 5/31/2016 1:06 PM, Jan Viktorin wrote: > 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). Comment added. Not const, as the *pool is set. >>>> +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... Done. >> >>>> +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? > > [...] Comments added. I've left as a void* for the moment. >>>> + >>>> +/** 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. I prefer the use of the word handler, unless others also have opinions either way? > >>>> +/** >>>> + * 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: [...] > So, it is possible to define those in the doc comment. Ah, I see now. My mistake, I assumed a different function. Added now. >> >>>> + */ >>>> +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. Yes, Added now to comments. > > [...] > >>>> >>>> + >>>> +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. For these handlers, the allocations are as per the original code before this patch. For new handlers, hardware allocators, stack allocators, etc. I've added comments on the 4 declarations. >> >>>> +#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. Will do. Rgds, Dave.