Re: [dpdk-dev] [PATCH] mempool: fix alignment of memzone length when populating

[Olivier Matz <olivier.matz@6wind.com>]

On Thu, May 03, 2018 at 12:34:59PM +0300, Andrew Rybchenko wrote:
> Hi Olivier,
> 
> On 05/02/2018 11:13 PM, Olivier Matz wrote:
> > When populating a mempool with the default function, if there is not
> > enough virtually contiguous memory for the whole mempool, it will be
> > populated with several chunks. A chunk of the maximum available length
> > is requested with:
> > 
> >    mz = rte_memzone_reserve_aligned(..., len=0, ..., align=x)
> > 
> > If align is smaller than the page size, the length of the memzone may
> > not be a multiple of the page size. This makes
> > rte_mempool_populate_virt() to fail because it requires a page-aligned
> > length. This patch forces the memzone length to be a multiple of page
> > size.
> > 
> > The problem can be reproduced easily by allocating more than available
> > memory:
> >    ./build/app/testpmd -l 0,1 -- --total-num-mbufs=65536
> >    ...
> >    Cause: Creation of mbuf pool for socket 0 failed: Invalid argument
> > 
> > After the patch, the error code is correct:
> >    ./build/app/testpmd -l 0,1 -- --total-num-mbufs=65536
> >    ...
> >    Cause: Creation of mbuf pool for socket 0 failed: Cannot allocate memory
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > Fixes: ba0009560c30 ("mempool: support new allocation methods")
> > ---
> > 
> > Hi Anatoly,
> > 
> > Another	option to fix this issue could be to ensure that
> > rte_memzone_reserve_aligned(..., len=0, ..., align=x) returns a length
> > that is	multiple of page size. Something like:
> > 
> >          mz = rte_memzone_reserve_aligned(mz_name, 0,
> >    -                       mp->socket_id, flags, align);
> >    +                       mp->socket_id, flags, RTE_MAX(pg_sz, align));
> 
> As far as I can see rte_mempool_populate_virt() checks that both address and
> length are page size aligned. So, I think both should be used. This one to
> be sure
> that address is page-aligned, below to ensure that length is page size
> aligned.
> May be one of them will be the property of the allocated region any way, but
> it is safer to guarantee both restrictions.

You are right, we should also ensure that the address is aligned, so
the second patch is needed. Will send a v2. Thanks.



> 
> > 
> > Let me know if you prefer this way.
> > 
> > Thanks,
> > Olivier
> > 
> > 
> >   lib/librte_mempool/rte_mempool.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> > index cf5d124ec..78c3e95ec 100644
> > --- a/lib/librte_mempool/rte_mempool.c
> > +++ b/lib/librte_mempool/rte_mempool.c
> > @@ -709,7 +709,7 @@ rte_mempool_populate_default(struct rte_mempool *mp)
> >   				(void *)(uintptr_t)mz);
> >   		else
> >   			ret = rte_mempool_populate_virt(mp, mz->addr,
> > -				mz->len, pg_sz,
> > +				RTE_ALIGN_FLOOR(mz->len, pg_sz), pg_sz,
> >   				rte_mempool_memchunk_mz_free,
> >   				(void *)(uintptr_t)mz);
> >   		if (ret < 0) {
>