Re: [dpdk-dev] [PATCH v1 2/9] mempool: add op to populate objects using provided memory

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

On Sat, Mar 10, 2018 at 03:39:35PM +0000, Andrew Rybchenko wrote:
> The callback allows to customize how objects are stored in the
> memory chunk. Default implementation of the callback which simply
> puts objects one by one is available.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

[...]

> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -99,7 +99,8 @@ static unsigned optimize_object_size(unsigned obj_size)
>  }
>  
>  static void
> -mempool_add_elem(struct rte_mempool *mp, void *obj, rte_iova_t iova)
> +mempool_add_elem(struct rte_mempool *mp, __rte_unused void *opaque,
> +		 void *obj, rte_iova_t iova)
>  {
>  	struct rte_mempool_objhdr *hdr;
>  	struct rte_mempool_objtlr *tlr __rte_unused;
> @@ -116,9 +117,6 @@ mempool_add_elem(struct rte_mempool *mp, void *obj, rte_iova_t iova)
>  	tlr = __mempool_get_trailer(obj);
>  	tlr->cookie = RTE_MEMPOOL_TRAILER_COOKIE;
>  #endif
> -
> -	/* enqueue in ring */
> -	rte_mempool_ops_enqueue_bulk(mp, &obj, 1);
>  }
>  
>  /* call obj_cb() for each mempool element */

Before this patch, the purpose of mempool_add_elem() was to add
an object into a mempool:
1- write object header and trailers
2- chain it into the list of objects
3- add it into the ring/stack/whatever (=enqueue)

Now, the enqueue is done in rte_mempool_op_populate_default() or will be
done in the driver. I'm not sure it's a good idea to separate 3- from
2-, because an object that is chained into the list is expected to be
in the ring/stack too.

This risk of mis-synchronization is also enforced by the fact that
ops->populate() can be provided by the driver and mempool_add_elem() is
passed as a callback pointer.

It's not clear to me why rte_mempool_ops_enqueue_bulk() is
removed from mempool_add_elem().



> @@ -396,16 +394,13 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
>  	else
>  		off = RTE_PTR_ALIGN_CEIL(vaddr, RTE_CACHE_LINE_SIZE) - vaddr;
>  
> -	while (off + total_elt_sz <= len && mp->populated_size < mp->size) {
> -		off += mp->header_size;
> -		if (iova == RTE_BAD_IOVA)
> -			mempool_add_elem(mp, (char *)vaddr + off,
> -				RTE_BAD_IOVA);
> -		else
> -			mempool_add_elem(mp, (char *)vaddr + off, iova + off);
> -		off += mp->elt_size + mp->trailer_size;
> -		i++;
> -	}
> +	if (off > len)
> +		return -EINVAL;

I think there is a memory leak here (memhdr), but it's my fault ;)
I introduced a similar code in commit 84121f1971:

      if (i == 0)
              return -EINVAL;

I can send a patch for it if you want.