From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 395132BA7 for ; Fri, 10 Jun 2016 11:34:41 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 10 Jun 2016 02:34:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,449,1459839600"; d="scan'208";a="825412030" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.22]) ([10.237.221.22]) by orsmga003.jf.intel.com with ESMTP; 10 Jun 2016 02:34:38 -0700 To: Olivier Matz , Jan Viktorin 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> <5756931C.70805@intel.com> <57593962.6010802@intel.com> <20160609150918.502322c8@pcviktorin.fit.vutbr.cz> <575A6C68.3090007@6wind.com> Cc: Shreyansh Jain , "dev@dpdk.org" , "jerin.jacob@caviumnetworks.com" From: "Hunt, David" Message-ID: <575A89AE.4070709@intel.com> Date: Fri, 10 Jun 2016 10:34:38 +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: <575A6C68.3090007@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jun 2016 09:34:41 -0000 Hi all, On 10/6/2016 8:29 AM, Olivier Matz wrote: > 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_(..., 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 would tend to agree, even though yesterday I proposed making that change. However, thinking about it some more, I'm not totally happy with the MEMPOOL_F_PKT_ALLOC addition. It adds yet another method of creating a mempool, and I think that may introduce some confusion with some developers. I also like the suggestion of rte_pktmbuf_pool_create_(..., name) suggested above, I was thinking the same myself last night, and I would prefer that rather than adding the MEMPOOL_F_PKT_ALLOC flag. Developers can add that function into their apps as a wrapper to rte_mempool_create_empty->rte_mempool_set_ops_byname should the need to have more than one pktmbuf allocator. Otherwise they can use the one that makes use of the RTE_MBUF_DEFAULT_MEMPOOL_OPS config setting. > 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