From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 71B8668F2 for ; Mon, 30 May 2016 13:27:29 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP; 30 May 2016 04:27:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,389,1459839600"; d="scan'208";a="965091822" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.36]) ([10.237.220.36]) by orsmga001.jf.intel.com with ESMTP; 30 May 2016 04:27:28 -0700 To: Jerin Jacob References: <1460642270-8803-1-git-send-email-olivier.matz@6wind.com> <1463665501-18325-1-git-send-email-david.hunt@intel.com> <1463665501-18325-2-git-send-email-david.hunt@intel.com> <20160524153509.GA11249@localhost.localdomain> <574818EA.7010806@intel.com> <20160527103311.GA13577@localhost.localdomain> <57485D4F.9020302@intel.com> <20160530094129.GA7963@localhost.localdomain> Cc: dev@dpdk.org, olivier.matz@6wind.com, yuanhan.liu@linux.intel.com, pmatilai@redhat.com From: "Hunt, David" Message-ID: <574C239E.8070705@intel.com> Date: Mon, 30 May 2016 12:27:26 +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: <20160530094129.GA7963@localhost.localdomain> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 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: Mon, 30 May 2016 11:27:29 -0000 On 5/30/2016 10:41 AM, Jerin Jacob wrote: --snip-- >> Of course, that won't help if we need to pass in more data, in which case >> we'd probably need an >> opaque data pointer somewhere. It would probably be most useful to pass it >> in with the >> alloc, which may need the data. Any suggestions? > But the top level rte_mempool_create() function needs to pass the data. Right? > That would be an ABI change. IMO, we need to start thinking about > passing a struct of config data to rte_mempool_create to create > backward compatibility on new argument addition to rte_mempool_create() New mempool handlers will use rte_mempool_create_empty(), rte_mempool_set_handler(), then rte_mempool_populate_*(). These three functions are new to this release, to no problem to add a parameter to one of them for the config data. Also since we're adding some new elements to the mempool structure, how about we add a new pointer for a void pointer to a config data structure, as defined by the handler. So, new element in rte_mempool struct alongside the *pool void *pool; void *pool_config; Then add a param to the rte_mempool_set_handler function: int rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void *pool_config) The function would simply set the pointer in the mempool struct, and the custom handler alloc/create function would use as apporopriate as needed. Handlers that do not need this data can be passed NULL. > Other points in HW assisted pool manager perspective, > > 1) May be RING can be replaced with some other higher abstraction name > for the internal MEMPOOL_F_RING_CREATED flag Agreed. I'll change to MEMPOOL_F_POOL_CREATED, since we're already changing the *ring element of the mempool struct to *pool > 2) IMO, It is better to change void *pool in struct rte_mempool to > anonymous union type, something like below, so that mempool > implementation can choose the best type. > union { > void *pool; > uint64_t val; > } Could we do this by using the union for the *pool_config suggested above, would that give you what you need? > 3) int32_t handler_idx creates 4 byte hole in struct rte_mempool in > 64 bit system. IMO it better to rearrange.(as const struct rte_memzone > *mz comes next) OK, Will look at this. > 4) IMO, It is better to change ext_alloc/ext_free to ext_create/ext_destroy > as their is no allocation in HW assisted pool manager case, > it will be mostly creating some HW initialization. OK, I'll change. I think that makes more sense. Regards, Dave.