From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 4DAD7A490 for ; Wed, 17 Jan 2018 16:04:15 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id C5872B80092; Wed, 17 Jan 2018 15:04:13 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Wed, 17 Jan 2018 15:03:58 +0000 To: Olivier MATZ CC: , "Artem V. Andreev" , "Santosh Shukla" References: <1511539591-20966-1-git-send-email-arybchenko@solarflare.com> <1511539591-20966-3-git-send-email-arybchenko@solarflare.com> <20171214133721.b7z2i7yccwpath37@platinum> From: Andrew Rybchenko Message-ID: Date: Wed, 17 Jan 2018 18:03:52 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20171214133721.b7z2i7yccwpath37@platinum> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23600.003 X-TM-AS-Result: No--19.959800-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1516201454-c4Y7qtWu4baL Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [RFC PATCH 2/6] mempool: implement clustered object allocation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jan 2018 15:04:15 -0000 On 12/14/2017 04:37 PM, Olivier MATZ wrote: > On Fri, Nov 24, 2017 at 04:06:27PM +0000, Andrew Rybchenko wrote: >> From: "Artem V. Andreev" >> >> Clustered allocation is required to simplify packaging objects into >> buckets and search of the bucket control structure by an object. >> >> Signed-off-by: Artem V. Andreev >> Signed-off-by: Andrew Rybchenko >> --- >> lib/librte_mempool/rte_mempool.c | 39 +++++++++++++++++++++++++++++++++++---- >> lib/librte_mempool/rte_mempool.h | 23 +++++++++++++++++++++-- >> test/test/test_mempool.c | 2 +- >> 3 files changed, 57 insertions(+), 7 deletions(-) >> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c >> index d50dba4..43455a3 100644 >> --- a/lib/librte_mempool/rte_mempool.c >> +++ b/lib/librte_mempool/rte_mempool.c >> @@ -239,7 +239,8 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags, >> */ >> size_t >> rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, >> - unsigned int flags) >> + unsigned int flags, >> + const struct rte_mempool_info *info) >> { >> size_t obj_per_page, pg_num, pg_sz; >> unsigned int mask; >> @@ -252,6 +253,17 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift, >> if (total_elt_sz == 0) >> return 0; >> >> + if (flags & MEMPOOL_F_CAPA_ALLOCATE_IN_CLUSTERS) { >> + unsigned int align_shift = >> + rte_bsf32( >> + rte_align32pow2(total_elt_sz * >> + info->cluster_size)); >> + if (pg_shift < align_shift) { >> + return ((elt_num / info->cluster_size) + 2) >> + << align_shift; >> + } >> + } >> + > +Cc Santosh for this > > To be honnest, that was my fear when introducing > MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS and MEMPOOL_F_CAPA_PHYS_CONTIG to see more > and more specific flags in generic code. > > I feel that the hidden meaning of these flags is more "if driver == foo", > which shows that something is wrong is the current design. > > We have to think about another way to do. Let me try to propose > something (to be deepen). > > The standard way to create a mempool is: > > mp = create_empty(...) > set_ops_by_name(mp, "my-driver") // optional > populate_default(mp) // or populate_*() > obj_iter(mp, callback, arg) // optional, to init objects > // and optional local func to init mempool priv > > First, we can consider deprecating some APIs like: > - rte_mempool_xmem_create() > - rte_mempool_xmem_size() > - rte_mempool_xmem_usage() > - rte_mempool_populate_iova_tab() > > These functions were introduced for xen, which was recently > removed. They are complex to use, and are not used anywhere else in > DPDK. > > Then, instead of having flags (quite hard to understand without knowing > the underlying driver), we can let the mempool drivers do the > populate_default() operation. For that we can add a populate_default > field in mempool ops. Same for populate_virt(), populate_anon(), and > populate_phys() which can return -ENOTSUP if this is not > implemented/implementable on a specific driver, or if flags > (NO_CACHE_ALIGN, NO_SPREAD, ...) are not supported. If the function > pointer is NULL, use the generic function. > > Thanks to this, the generic code would remain understandable and won't > have to care about how memory should be allocated for a specific driver. > > Thoughts? Yes, I agree. This week we'll provide updated version of the RFC which covers it including transition of the mempool/octeontx. I think it is sufficient to introduce two new ops:  1. To calculate memory space required to store specified number of objects  2. To populate objects in the provided memory chunk (the op will be called      from rte_mempool_populate_iova() which is a leaf function for all      rte_mempool_populate_*() calls. It will allow to avoid duplication and keep memchunks housekeeping inside mempool library. > [...] > >> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h >> index 3c59d36..9bcb8b7 100644 >> --- a/lib/librte_mempool/rte_mempool.h >> +++ b/lib/librte_mempool/rte_mempool.h >> @@ -220,7 +220,10 @@ struct rte_mempool_memhdr { >> /* >> * Additional information about the mempool >> */ >> -struct rte_mempool_info; >> +struct rte_mempool_info { >> + /** Number of objects in a cluster */ >> + unsigned int cluster_size; >> +}; > I think what I'm proposing would also prevent to introduce this > structure, which is generic but only applies to this driver. Yes