From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH v7 3/5] lib/mempool: allow page size aligned mempool Date: Fri, 29 Mar 2019 18:42:48 +0100 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=us-ascii 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 To: Andrew Rybchenko Return-path: 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) Content-Disposition: inline In-Reply-To: <7356c3b8-94d0-1b15-662b-09fd345f0f45@solarflare.com> 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, 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