From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id F2812A05D3 for ; Fri, 29 Mar 2019 18:42:59 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C13C02965; Fri, 29 Mar 2019 18:42:57 +0100 (CET) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 4EC74A3 for ; Fri, 29 Mar 2019 18:42:56 +0100 (CET) Received: from lfbn-1-5920-128.w90-110.abo.wanadoo.fr ([90.110.126.128] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h9vZ6-0002mB-H5; Fri, 29 Mar 2019 18:45:17 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 29 Mar 2019 18:42:48 +0100 Date: Fri, 29 Mar 2019 18:42:48 +0100 From: Olivier Matz To: Andrew Rybchenko Cc: Xiaolong Ye , dev@dpdk.org, David Marchand , Qi Zhang , Karlsson Magnus , Topel Bjorn , Maxime Coquelin , Stephen Hemminger , Ferruh Yigit , Luca Boccassi , Bruce Richardson , Ananyev Konstantin Message-ID: <20190329174248.2nx5udij6md4bome@platinum> References: <20190301080947.91086-1-xiaolong.ye@intel.com> <20190327090027.72170-1-xiaolong.ye@intel.com> <20190327090027.72170-4-xiaolong.ye@intel.com> <7356c3b8-94d0-1b15-662b-09fd345f0f45@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <7356c3b8-94d0-1b15-662b-09fd345f0f45@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v7 3/5] lib/mempool: allow page size aligned mempool 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190329174248.hAHh5F_gSQo6vqKUnNoFH4k3Y8_5ElsjZczfLKNjPOY@z> Hi, On Fri, Mar 29, 2019 at 01:37:17PM +0300, Andrew Rybchenko wrote: > On 3/27/19 12:00 PM, Xiaolong Ye wrote: > > Allow create a mempool with page size aligned base address. > > > > Signed-off-by: Qi Zhang > > Signed-off-by: Xiaolong Ye > > --- > > lib/librte_mempool/rte_mempool.c | 3 +++ > > lib/librte_mempool/rte_mempool.h | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > > index 683b216f9..cfbb49ea5 100644 > > --- a/lib/librte_mempool/rte_mempool.c > > +++ b/lib/librte_mempool/rte_mempool.c > > @@ -543,6 +543,9 @@ rte_mempool_populate_default(struct rte_mempool *mp) > > if (try_contig) > > flags |= RTE_MEMZONE_IOVA_CONTIG; > > + if (mp->flags & MEMPOOL_CHUNK_F_PAGE_ALIGN) > > + align = RTE_MAX(align, (size_t)getpagesize()); > > + > > mz = rte_memzone_reserve_aligned(mz_name, mem_size, > > mp->socket_id, flags, align); > > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > > index 7c9cd9a2f..47729f7c9 100644 > > --- a/lib/librte_mempool/rte_mempool.h > > +++ b/lib/librte_mempool/rte_mempool.h > > @@ -264,6 +264,7 @@ struct rte_mempool { > > #define MEMPOOL_F_POOL_CREATED 0x0010 /**< Internal: pool is created. */ > > #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */ > > #define MEMPOOL_F_NO_PHYS_CONTIG MEMPOOL_F_NO_IOVA_CONTIG /* deprecated */ > > +#define MEMPOOL_CHUNK_F_PAGE_ALIGN 0x0040 /**< Chunk's base address is page aligned */ > > Now the define name better explains what happens. I would name it > MEMPOOL_F_CHUNK_PAGE_ALIGN to have MEMPOOL_F_ prefix for all flags, > but it is minor. More important is what is behind and I have doubts that it > is > a right way to go. > > I have already asked what is the final goal (but agree that question is > unclear). > Personally I doubt that the final goal is just having chunk page aligned. > > It looks like the patch makes an assumption on how chunk is sliced into > objects/elements and the property of chunk address allows to achieve some > goal with elements start address. It looks very fragile and easily breakable > with other flags (or no flags). > > Also prefix should be just "mempool:", not "lib/mempool". +1 I agree with Andrew. Please see my comment in patch 4. Regards, Olivier