[dpdk-dev] mempool: fix virt address mempool population

  Currently, populate_virt will check if mempool is already populated.
This will cause inability to reserve multi-chunk mempools if
contiguous memory is not a hard requirement, because if allocating
all-contiguous memory fails, mempool will retry with virtual addresses
and will call populate_virt. It seems that the original code never
anticipated more than one non-physically contiguous area.

Fix it by removing the check in populate virt. populate_anon() function
calls populate_virt() also, and it can be reasonably inferred that it is
expecting that virtual area is not already populated. Even though a
similar check is already in place there, also add the check that was
part of populate_virt() just in case.

Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")
Cc: olivier.matz@6wind.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_mempool/rte_mempool.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
  

14/05/2018 18:06, Anatoly Burakov:
> Currently, populate_virt will check if mempool is already populated.
> This will cause inability to reserve multi-chunk mempools if
> contiguous memory is not a hard requirement, because if allocating
> all-contiguous memory fails, mempool will retry with virtual addresses
> and will call populate_virt. It seems that the original code never
> anticipated more than one non-physically contiguous area.
> 
> Fix it by removing the check in populate virt. populate_anon() function
> calls populate_virt() also, and it can be reasonably inferred that it is
> expecting that virtual area is not already populated. Even though a
> similar check is already in place there, also add the check that was
> part of populate_virt() just in case.
> 
> Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")

If it is really the root cause, you need to add Cc: stable@dpdk.org.

> Cc: olivier.matz@6wind.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

This patch looks sensible. It will wait some review,
so it is a candidate for -rc4.
  

On 14-May-18 7:48 PM, Thomas Monjalon wrote:
> 14/05/2018 18:06, Anatoly Burakov:
>> Currently, populate_virt will check if mempool is already populated.
>> This will cause inability to reserve multi-chunk mempools if
>> contiguous memory is not a hard requirement, because if allocating
>> all-contiguous memory fails, mempool will retry with virtual addresses
>> and will call populate_virt. It seems that the original code never
>> anticipated more than one non-physically contiguous area.
>>
>> Fix it by removing the check in populate virt. populate_anon() function
>> calls populate_virt() also, and it can be reasonably inferred that it is
>> expecting that virtual area is not already populated. Even though a
>> similar check is already in place there, also add the check that was
>> part of populate_virt() just in case.
>>
>> Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")
> 
> If it is really the root cause, you need to add Cc: stable@dpdk.org.

Yes. It's not as pressing for previous DPDK versions because this 
codepath wasn't triggered unless --no-huge was used, but it's still a bug.

> 
>> Cc: olivier.matz@6wind.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> This patch looks sensible. It will wait some review,
> so it is a candidate for -rc4.
> 
> 
> 
> 
>
  

On Tue, May 15, 2018 at 08:58:06AM +0100, Burakov, Anatoly wrote:
> On 14-May-18 7:48 PM, Thomas Monjalon wrote:
> > 14/05/2018 18:06, Anatoly Burakov:
> > > Currently, populate_virt will check if mempool is already populated.
> > > This will cause inability to reserve multi-chunk mempools if
> > > contiguous memory is not a hard requirement, because if allocating
> > > all-contiguous memory fails, mempool will retry with virtual addresses
> > > and will call populate_virt. It seems that the original code never
> > > anticipated more than one non-physically contiguous area.
> > > 
> > > Fix it by removing the check in populate virt. populate_anon() function
> > > calls populate_virt() also, and it can be reasonably inferred that it is
> > > expecting that virtual area is not already populated. Even though a
> > > similar check is already in place there, also add the check that was
> > > part of populate_virt() just in case.
> > > 
> > > Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")

Acked-by: Olivier Matz <olivier.matz@6wind.com>

> > If it is really the root cause, you need to add Cc: stable@dpdk.org.
> 
> Yes. It's not as pressing for previous DPDK versions because this codepath
> wasn't triggered unless --no-huge was used, but it's still a bug.

Agree we should queue it for stable, even if it's not that critical.

Thanks,
Olivier
  

On 05/14/2018 07:06 PM, Anatoly Burakov wrote:
> Currently, populate_virt will check if mempool is already populated.
> This will cause inability to reserve multi-chunk mempools if
> contiguous memory is not a hard requirement, because if allocating
> all-contiguous memory fails, mempool will retry with virtual addresses
> and will call populate_virt. It seems that the original code never
> anticipated more than one non-physically contiguous area.
>
> Fix it by removing the check in populate virt. populate_anon() function
> calls populate_virt() also, and it can be reasonably inferred that it is
> expecting that virtual area is not already populated. Even though a
> similar check is already in place there, also add the check that was
> part of populate_virt() just in case.
>
> Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")
> Cc: olivier.matz@6wind.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_mempool/rte_mempool.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 9f1a425..8c8b9f8 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -492,9 +492,6 @@ rte_mempool_populate_virt(struct rte_mempool *mp, char *addr,
>   	size_t off, phys_len;
>   	int ret, cnt = 0;
>   
> -	/* mempool must not be populated */
> -	if (mp->nb_mem_chunks != 0)
> -		return -EEXIST;
>   	/* address and len must be page-aligned */
>   	if (RTE_PTR_ALIGN_CEIL(addr, pg_sz) != addr)
>   		return -EINVAL;
> @@ -771,7 +768,7 @@ rte_mempool_populate_anon(struct rte_mempool *mp)
>   	char *addr;
>   
>   	/* mempool is already populated, error */
> -	if (!STAILQ_EMPTY(&mp->mem_list)) {
> +	if ((!STAILQ_EMPTY(&mp->mem_list)) || mp->nb_mem_chunks != 0) {

I've seen the description above.
I see no point to add the check here since it is basically duplication.
nb_mem_chunks is incremented when chunk is added to mem_list and
decremented when chunk is removed from mem_list.
However, it is not a problem anyway. So, OK to go as is.

>   		rte_errno = EINVAL;
>   		return 0;
>   	}

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
  

15/05/2018 13:28, Olivier Matz:
> On Tue, May 15, 2018 at 08:58:06AM +0100, Burakov, Anatoly wrote:
> > On 14-May-18 7:48 PM, Thomas Monjalon wrote:
> > > 14/05/2018 18:06, Anatoly Burakov:
> > > > Currently, populate_virt will check if mempool is already populated.
> > > > This will cause inability to reserve multi-chunk mempools if
> > > > contiguous memory is not a hard requirement, because if allocating
> > > > all-contiguous memory fails, mempool will retry with virtual addresses
> > > > and will call populate_virt. It seems that the original code never
> > > > anticipated more than one non-physically contiguous area.
> > > > 
> > > > Fix it by removing the check in populate virt. populate_anon() function
> > > > calls populate_virt() also, and it can be reasonably inferred that it is
> > > > expecting that virtual area is not already populated. Even though a
> > > > similar check is already in place there, also add the check that was
> > > > part of populate_virt() just in case.
> > > > 
> > > > Fixes: aab4f62d6c1c ("mempool: support no hugepage mode")
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> > > If it is really the root cause, you need to add Cc: stable@dpdk.org.
> > 
> > Yes. It's not as pressing for previous DPDK versions because this codepath
> > wasn't triggered unless --no-huge was used, but it's still a bug.
> 
> Agree we should queue it for stable, even if it's not that critical.

Applied, thanks