From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 6F7A21D8E for ; Thu, 14 Dec 2017 14:37:31 +0100 (CET) Received: from lfbn-lil-1-110-231.w90-45.abo.wanadoo.fr ([90.45.197.231] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1ePTnh-0007Yy-HU; Thu, 14 Dec 2017 14:43:50 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Thu, 14 Dec 2017 14:37:24 +0100 Date: Thu, 14 Dec 2017 14:37:24 +0100 From: Olivier MATZ To: Andrew Rybchenko Cc: dev@dpdk.org, "Artem V. Andreev" , Santosh Shukla Message-ID: <20171214133721.b7z2i7yccwpath37@platinum> References: <1511539591-20966-1-git-send-email-arybchenko@solarflare.com> <1511539591-20966-3-git-send-email-arybchenko@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1511539591-20966-3-git-send-email-arybchenko@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) 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: Thu, 14 Dec 2017 13:37:31 -0000 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? [...] > 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.