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

Message ID 20190327090027.72170-4-xiaolong.ye@intel.com
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • Introduce AF_XDP PMD
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ye Xiaolong March 27, 2019, 9 a.m. UTC
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(+)

Comments

Ferruh Yigit March 28, 2019, 7:34 p.m. UTC | #1
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
Andrew Rybchenko March 29, 2019, 10:37 a.m. UTC | #2
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.
Olivier Matz March 29, 2019, 5:42 p.m. UTC | #3
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

Patch
diff mbox series

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 */
 
 /**
  * @internal When debug is enabled, store some statistics.