RE: [PATCH v1 2/3] mempool: skip unpopulated pools in walk and dump

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Robert Sanford <rsanford@akamai.com>
> 
> rte_mempool_create_empty() adds a mempool to the mempool list before
> the mempool has a store. The store is allocated, and
> RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
> between, rte_mempool_walk() passes the mempool to its callback, and
> rte_mempool_list_dump() calls rte_mempool_dump() on it, which reads
> the number of objects in the store. With the ring driver this
> dereferences a NULL ring, and the process crashes.
> 
> Skip mempools that do not have RTE_MEMPOOL_F_POOL_CREATED set in
> rte_mempool_walk() and rte_mempool_list_dump(), and document it.

NAK to rte_mempool_walk() skipping unpopulated pools. The effect is too broad.

The correct solution is for the relevant callback functions to check RTE_MEMPOOL_F_POOL_CREATED, if it needs to access the mempool driver.

Instead, please fix rte_mempool_dump(FILE *f, struct rte_mempool *mp) to not dump the parts accessing the mempool driver when RTE_MEMPOOL_F_POOL_CREATED is not set. We want to know that the mempool exists, and partial information is much better than no information.

> The
> populating thread sets the flag without holding a lock, so add a
> release fence before setting it and an acquire fence after checking
> it. A walker that sees the flag then also sees the store.
> 
> rte_mempool_lookup() is unchanged. It does not access the store, and
> callers may look up a mempool that is not populated yet.
> 
> Fixes: 449c49b93a6b ("mempool: support handler operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>  lib/mempool/rte_mempool.c | 20 +++++++++++++++++++-
>  lib/mempool/rte_mempool.h |  3 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index d9f339a3c2..7c4d4697c7 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -304,6 +304,12 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
>  		ret = rte_mempool_ops_alloc(mp);
>  		if (ret != 0)
>  			return ret;
> +		/*
> +		 * rte_mempool_walk() and rte_mempool_list_dump() access
> the
> +		 * store of pools with this flag set. Make the store
> visible
> +		 * before the flag.
> +		 */
> +		rte_atomic_thread_fence(rte_memory_order_release);
>  		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
>  	}
>  	return 0;
> @@ -1412,6 +1418,11 @@ rte_mempool_list_dump(FILE *f)
> 
>  	TAILQ_FOREACH(te, mempool_list, next) {
>  		mp = (struct rte_mempool *) te->data;
> +		/* skip mempools that are not populated yet */
> +		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
> +			continue;
> +		/* pairs with the release fence in mempool_ops_alloc_once()
> */
> +		rte_atomic_thread_fence(rte_memory_order_acquire);
>  		rte_mempool_dump(f, mp);
>  	}
> 
> @@ -1460,7 +1471,14 @@ void rte_mempool_walk(void (*func)(struct
> rte_mempool *, void *),
>  	rte_mcfg_mempool_read_lock();
> 
>  	RTE_TAILQ_FOREACH_SAFE(te, mempool_list, next, tmp_te) {
> -		(*func)((struct rte_mempool *) te->data, arg);
> +		struct rte_mempool *mp = (struct rte_mempool *) te->data;
> +
> +		/* skip mempools that are not populated yet */
> +		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
> +			continue;
> +		/* pairs with the release fence in mempool_ops_alloc_once()
> */
> +		rte_atomic_thread_fence(rte_memory_order_acquire);
> +		(*func)(mp, arg);
>  	}
> 
>  	rte_mcfg_mempool_read_unlock();
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index bd58d8a9d5..7782768a2d 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -1907,6 +1907,8 @@ static inline void *rte_mempool_get_priv(struct
> rte_mempool *mp)
>  /**
>   * Dump the status of all mempools on the console
>   *
> + * Mempools that are not populated yet are skipped.
> + *
>   * @param f
>   *   A pointer to a file for output
>   */
> @@ -1949,6 +1951,7 @@ uint32_t rte_mempool_calc_obj_size(uint32_t
> elt_size, uint32_t flags,
>   *
>   * The mempool list is locked while the iterator function runs, so the
>   * iterator function must not create or free a mempool.
> + * Mempools that are not populated yet are skipped.
>   *
>   * @param func
>   *   Iterator function
> --
> 2.34.1