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 B1AE0293B for ; Fri, 10 Jun 2016 11:02:50 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP; 10 Jun 2016 02:02:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,449,1459839600"; d="scan'208";a="984719641" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.22]) ([10.237.221.22]) by fmsmga001.fm.intel.com with ESMTP; 10 Jun 2016 02:02:41 -0700 To: Jan Viktorin , Olivier Matz 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> <20160610104952.55997f4d@pcviktorin.fit.vutbr.cz> Cc: Shreyansh Jain , "dev@dpdk.org" , "jerin.jacob@caviumnetworks.com" From: "Hunt, David" Message-ID: <575A8230.5060603@intel.com> Date: Fri, 10 Jun 2016 10:02:40 +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: <20160610104952.55997f4d@pcviktorin.fit.vutbr.cz> 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:02:51 -0000 Hi Jan, On 10/6/2016 9:49 AM, Jan Viktorin wrote: > On Fri, 10 Jun 2016 09:29:44 +0200 > 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. > Yes, I consider it quite strange. When thinking more about the mempool API, > I'd move the flags processing to the rte_mempool_create. Semantically, it > makes more sense as the "empty" clearly describes that it is empty. But with > the flags, it is not... What is the reason to have those flags there? Yes, they should be in rte_mempool_create. There were in an earlier patch, but regressed. I'll have them in rte_mempool_create in the next patch. [...] Rgds, Dave.