From mboxrd@z Thu Jan 1 00:00:00 1970 From: Santosh Shukla Subject: Re: [PATCH v2] mempool: Introduce _populate_mz_range api Date: Tue, 7 Feb 2017 09:30:01 +0530 Message-ID: <20170207035959.GA23401@santosh-Latitude-E5530-non-vPro> References: <1484922017-26030-1-git-send-email-santosh.shukla@caviumnetworks.com> <1484925221-18431-1-git-send-email-santosh.shukla@caviumnetworks.com> <20170131113151.3f8e07a0@platinum> <20170131143211.GA31464@santosh-Latitude-E5530-non-vPro> <20170206180115.480e07af@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , , , To: Olivier Matz Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0055.outbound.protection.outlook.com [104.47.34.55]) by dpdk.org (Postfix) with ESMTP id ECF869E3 for ; Tue, 7 Feb 2017 05:00:24 +0100 (CET) Content-Disposition: inline In-Reply-To: <20170206180115.480e07af@platinum> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Olivier, On Mon, Feb 06, 2017 at 06:01:15PM +0100, Olivier Matz wrote: > On Tue, 31 Jan 2017 20:02:26 +0530, Santosh Shukla > wrote: > > Hi Olivier, > > > > Reply inline. > > > > On Tue, Jan 31, 2017 at 11:31:51AM +0100, Olivier Matz wrote: > > > Hi Santosh, > > > > > > I guess this patch is targeted for 17.05, right? > > > > > > > Yes. > > > > > Please see some other comments below. > > > > > > On Fri, 20 Jan 2017 20:43:41 +0530, > > > wrote: > > > > From: Santosh Shukla [snip..] > > > > --- a/lib/librte_mempool/rte_mempool.h > > > > +++ b/lib/librte_mempool/rte_mempool.h > > > > @@ -387,6 +387,12 @@ typedef int (*rte_mempool_dequeue_t)(struct > > > > rte_mempool *mp, */ > > > > typedef unsigned (*rte_mempool_get_count)(const struct > > > > rte_mempool *mp); +/** > > > > + * Set the memzone va/pa addr range and len in the external pool. > > > > + */ > > > > +typedef void (*rte_mempool_populate_mz_range_t)(struct > > > > rte_mempool *mp, > > > > + const struct rte_memzone *mz); > > > > + > > > > > > And this API would be: > > > typedef void (*rte_mempool_populate_t)(struct rte_mempool *mp, > > > char *vaddr, phys_addr_t paddr, size_t len) > > > > > > > > > If your hw absolutly needs a contiguous memory, a solution could be: > > > > > > - add a new flag MEMPOOL_F_CONTIG (maybe a better nale could be > > > found) saying that all the mempool objects must be contiguous. > > > - add the ops_populate() callback in rte_mempool_populate_phys(), as > > > suggested above > > > > > > Then: > > > > > > /* create an empty mempool */ > > > rte_mempool_create_empty(...); > > > > > > /* set the handler: > > > * in the ext handler, the mempool flags are updated with > > > * MEMPOOL_F_CONTIG > > > */ > > > rte_mempool_set_ops_byname(..., "my_hardware"); > > > > > > > You mean, ext handler will set mempool flag using 'pool_config' > > param; somethng like rte_mempool_ops_by_name(..., "my_hardware" , > > MEMPOOL_F_CONTIG); ? > > I don't mean changing the API of rte_mempool_set_ops_byname(). > I was suggesting something like this: > > static int your_handler_alloc(struct rte_mempool *mp) > { > /* handler init */ > ... > > mp->flags |= MEMPOOL_F_CONTIG; > return 0; > } > Ok,. Will do in v3. > And update rte_mempool_populate_*() functions to manage this flag: > instead of segment, just fail if it cannot fit in one segment. It won't > really solve the issue, but at least it will be properly detected, and > the user could take dispositions to have more contiguous memory (ex: > use larger hugepages, allocate hugepages at boot time). > Agree. Will take care in v3. > > > > > /* if MEMPOOL_F_CONTIG is set, all populate() functions should > > > ensure > > > * that there is only one contiguous zone > > > */ > > > rte_mempool_populate_default(...); > > > > > > > I am not too sure about scope of MEMPOOL_F_CONTIG. How > > MEMPOOL_F_CONTIG flag {setted by application/ driver by calling > > rte_mempool_create(..., flag)} can make sure only one contiguous > > zone? Like to understand from you. > > As described above, there would be no change from application point of > view. The handler would set the mempool flag by itself to change the > behavior of the populate functions. > > Right. > > > > In my understanding: > > rte_mempool_populate_default() will request for memzone based on > > mp->size value; And if mp->size more than one hugepage_sz{i.e. one mz > > request not enough} then it will request more hugepages{ i.e. more mz > > request},. So IIIU then contiguity not gauranteed. > > Yes, that's how it works today. As EAL cannot guarantees that the > hugepages are physically contiguous, it tries to segment the mempool > objects in several memory zones. > > > Regards, > Olivier > > > Regards, Santosh