From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 508F92BB9 for ; Fri, 27 May 2016 16:44:34 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 27 May 2016 07:44:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,374,1459839600"; d="scan'208";a="990028060" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.26]) ([10.237.220.26]) by fmsmga002.fm.intel.com with ESMTP; 27 May 2016 07:44:32 -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> Cc: dev@dpdk.org, olivier.matz@6wind.com, yuanhan.liu@linux.intel.com, pmatilai@redhat.com From: "Hunt, David" Message-ID: <57485D4F.9020302@intel.com> Date: Fri, 27 May 2016 15:44:31 +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: <20160527103311.GA13577@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: Fri, 27 May 2016 14:44:34 -0000 On 5/27/2016 11:33 AM, Jerin Jacob wrote: > On Fri, May 27, 2016 at 10:52:42AM +0100, Hunt, David wrote: >> >> On 5/24/2016 4:35 PM, Jerin Jacob wrote: >>> On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote: >>>> + /* >>>> + * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to >>>> + * set the correct index into the handler table. >>>> + */ >>>> + if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) >>>> + rte_mempool_set_handler(mp, "ring_sp_sc"); >>>> + else if (flags & MEMPOOL_F_SP_PUT) >>>> + rte_mempool_set_handler(mp, "ring_sp_mc"); >>>> + else if (flags & MEMPOOL_F_SC_GET) >>>> + rte_mempool_set_handler(mp, "ring_mp_sc"); >>>> + else >>>> + rte_mempool_set_handler(mp, "ring_mp_mc"); >>> IMO, We should decouple the implementation specific flags of _a_ >>> external pool manager implementation from the generic rte_mempool_create_empty >>> function as going further when we introduce new flags for custom HW accelerated >>> external pool manager then this common code will be bloated. >> These flags are only there to maintain backward compatibility for the >> default handlers. I would not >> envisage adding more flags to this, I would suggest just adding a new >> handler using the new API calls. >> So I would not see this code growing much in the future. > IMHO, For _each_ HW accelerated external pool manager we may need to introduce > specific flag to tune to specific use cases.i.e MEMPOOL_F_* flags for > this exiting pool manager implemented in SW. For instance, when we add > a new HW external pool manager we may need to add MEMPOOL_MYHW_DONT_FREE_ON_SEND > (just a random name) to achieve certain functionally. > > So I propose let "unsigned flags" in mempool create to be the opaque type and each > external pool manager can define what it makes sense to that specific > pool manager as there is NO other means to configure the pool manager. > > For instance, on HW accelerated pool manager, the flag MEMPOOL_F_SP_PUT may > not make much sense as it can work with MP without any additional > settings in HW. > > So instead of adding these checks in common code, IMO, lets move this > to a pool manager specific "function pointer" function and invoke > the function pointer from generic mempool create function. > > What do you think? > > Jerin Jerin, That chunk of code above would be better moved all right. I'd suggest moving it to the rte_mempool_create function, as that's the one that needs the backward compatibility. On the flags issue, each mempool handler can re-interpret the flags as needed. Maybe we could use the upper half of the bits for different handlers, changing the meaning of the bits depending on which handler is being set up. We can then keep the lower half for bits that are common across all handlers? That way the user can just set the bits they are interested in for that handler. Also, the alloc function has access to the flags, so maybe the handler specific setup could be handled in the alloc function rather than adding a new function pointer? 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? Regards, Dave.