From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <olivier.matz@6wind.com> Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id A30562BA6 for <dev@dpdk.org>; Fri, 10 Jun 2016 09:29:53 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from <olivier.matz@6wind.com>) id 1bBGvI-0003Qd-Bs; Fri, 10 Jun 2016 09:32:08 +0200 To: Jan Viktorin <viktorin@rehivetech.com>, "Hunt, David" <david.hunt@intel.com> References: <1464874043-67467-1-git-send-email-david.hunt@intel.com> <1464965906-108927-1-git-send-email-david.hunt@intel.com> <1464965906-108927-2-git-send-email-david.hunt@intel.com> <DB5PR0401MB20545CC8494FB0965C338EE2905C0@DB5PR0401MB2054.eurprd04.prod.outlook.com> <5756931C.70805@intel.com> <DB5PR0401MB2054E6778695E327F48AE6E8905E0@DB5PR0401MB2054.eurprd04.prod.outlook.com> <57593962.6010802@intel.com> <20160609150918.502322c8@pcviktorin.fit.vutbr.cz> Cc: Shreyansh Jain <shreyansh.jain@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>, "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com> From: Olivier Matz <olivier.matz@6wind.com> Message-ID: <575A6C68.3090007@6wind.com> Date: Fri, 10 Jun 2016 09:29:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160609150918.502322c8@pcviktorin.fit.vutbr.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Fri, 10 Jun 2016 07:29:53 -0000 Hi, On 06/09/2016 03:09 PM, Jan Viktorin wrote: >>> My suggestion is to have an additional flag, >>> 'MEMPOOL_F_PKT_ALLOC', which, if specified, would: >>> >>> ... #define MEMPOOL_F_SC_GET 0x0008 #define >>> MEMPOOL_F_PKT_ALLOC 0x0010 ... >>> >>> in rte_mempool_create_empty: ... after checking the other >>> MEMPOOL_F_* flags... >>> >>> if (flags & MEMPOOL_F_PKT_ALLOC) rte_mempool_set_ops_byname(mp, >>> RTE_MBUF_DEFAULT_MEMPOOL_OPS) >>> >>> And removing the redundant call to rte_mempool_set_ops_byname() >>> in rte_pktmbuf_create_pool(). >>> >>> Thereafter, rte_pktmbuf_pool_create can be changed to: >>> >>> ... mp = rte_mempool_create_empty(name, n, elt_size, cache_size, >>> - sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); >>> + sizeof(struct rte_pktmbuf_pool_private), socket_id, + >>> MEMPOOL_F_PKT_ALLOC); if (mp == NULL) return NULL; >> >> Yes, this would simplify somewhat the creation of a pktmbuf pool, >> in that it replaces the rte_mempool_set_ops_byname with a flag bit. >> However, I'm not sure we want to introduce a third method of >> creating a mempool to the developers. If we introduced this, we >> would then have: 1. rte_pktmbuf_pool_create() 2. >> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which >> would use the configured custom handler) 3. >> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set >> followed by a call to rte_mempool_set_ops_byname() (would allow >> several different custom handlers to be used in one application >> >> Does anyone else have an opinion on this? Oliver, Jerin, Jan? > > I am quite careful about this topic as I don't feel to be very > involved in all the use cases. My opinion is that the _new API_ > should be able to cover all cases and the _old API_ should be > backwards compatible, however, built on top of the _new API_. > > I.e. I think, the flags MEMPOOL_F_SP_PUT, MEMPOOL_F_SC_GET (relicts > of the old API) should be accepted by the old API ONLY. The > rte_mempool_create_empty should not process them. The rte_mempool_create_empty() function already processes these flags (SC_GET, SP_PUT) as of today. > Similarly for a potential MEMPOOL_F_PKT_ALLOC, I would not polute the > rte_mempool_create_empty by this anymore. +1 I think we should stop adding flags. Flags are prefered for independent features. Here, what would be the behavior with MEMPOOL_F_PKT_ALLOC + MEMPOOL_F_SP_PUT? Another reason to not add this flag is the rte_mempool library should not be aware of mbufs. The mbuf pools rely on mempools, but not the contrary. > In overall we would get exactly 2 approaches (and not more): > > * using rte_mempool_create with flags calling the > rte_mempool_create_empty and rte_mempool_set_ops_byname internally > (so this layer can be marked as deprecated and removed in the > future) Agree. This was one of the objective of my mempool rework patchset: provide a more flexible API, and avoid functions with 10 to 15 arguments. > * using rte_mempool_create_empty + rte_mempool_set_ops_byname - > allowing any customizations but with the necessity to change the > applications (new preferred API) Yes. And if required, maybe a third API is possible in case of mbuf pools. Indeed, the applications are encouraged to use rte_pktmbuf_pool_create() to create a pool of mbuf instead of mempool API. If an application wants to select specific ops for it, we could add: rte_pktmbuf_pool_create_<something>(..., name) instead of using the mempool API. I think this is what Shreyansh suggests when he says: It sounds fine if calls to rte_mempool_* can select an external handler *optionally* - but, if we pass it as command line, it would be binding (at least, semantically) for rte_pktmbuf_* calls as well. Isn't it? > So, the old applications can stay as they are (OK, with a possible > new flag MEMPOOL_F_PKT_ALLOC) and the new one can do the same but you > have to set the ops explicitly. > > The more different ways of using those APIs we have, the greater hell > we have to maintain. I'm really not in favor of a MEMPOOL_F_PKT_ALLOC flag in mempool api. I think David's patch is already a good step forward. Let's do it step by step. Next step is maybe to update some applications (at least testpmd) to select a new pool handler dynamically. Regards, Olivier