[v7,3/5] lib/mempool: allow page size aligned mempool

  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(+)
  

On 3/27/2019 9:00 AM, 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>


Hi Andrew, Olivier,

Can you please check this patch,
It is adding a new mempool flag which is dependency for the pmd.

Thanks,
ferruh
  

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.
  

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 <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".

+1
I agree with Andrew. Please see my comment in patch 4.


Regards,
Olivier