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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.

>
> 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) {