From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Rybchenko <arybchenko@solarflare.com>
Subject: Re: [PATCH v7 3/5] lib/mempool: allow page size aligned
 mempool
Date: Fri, 29 Mar 2019 13:37:17 +0300
Message-ID: <7356c3b8-94d0-1b15-662b-09fd345f0f45@solarflare.com>
References: <20190301080947.91086-1-xiaolong.ye@intel.com>
 <20190327090027.72170-1-xiaolong.ye@intel.com>
 <20190327090027.72170-4-xiaolong.ye@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Qi Zhang <qi.z.zhang@intel.com>, Karlsson Magnus
 <magnus.karlsson@intel.com>, Topel Bjorn <bjorn.topel@intel.com>, "Maxime
 Coquelin" <maxime.coquelin@redhat.com>, Stephen Hemminger
 <stephen@networkplumber.org>, Ferruh Yigit <ferruh.yigit@intel.com>, "Luca
 Boccassi" <bluca@debian.org>, Bruce Richardson <bruce.richardson@intel.com>,
 Ananyev Konstantin <konstantin.ananyev@intel.com>
To: Xiaolong Ye <xiaolong.ye@intel.com>, <dev@dpdk.org>, David Marchand
 <david.marchand@redhat.com>
Return-path: <dev-bounces@dpdk.org>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
 [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 4584E343C
 for <dev@dpdk.org>; Fri, 29 Mar 2019 11:37:31 +0100 (CET)
In-Reply-To: <20190327090027.72170-4-xiaolong.ye@intel.com>
Content-Language: en-GB
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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 <qi.z.zhang@intel.com>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
>   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".

Andrew.