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 <arybche...@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.
