RE: [PATCH v2] mempool cache: add zero-copy get and put functions

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

> From: Kamalakshitha Aligeri [mailto:Kamalakshitha.Aligeri@arm.com]
> Sent: Tuesday, 29 November 2022 21.54
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Wednesday, November 16, 2022 12:04 PM
> >
> > Zero-copy access to mempool caches is beneficial for PMD performance,
> and
> > must be provided by the mempool library to fix [Bug 1052] without a
> > performance regression.
> >
> > [Bug 1052]: https://bugs.dpdk.org/show_bug.cgi?id=1052
> >
> > v2:
> > * Fix checkpatch warnings.
> > * Fix missing registration of trace points.
> > * The functions are inline, so they don't go into the map file.
> > v1 changes from the RFC:
> > * Removed run-time parameter checks. (Honnappa)
> >   This is a hot fast path function; requiring correct application
> >   behaviour, i.e. function parameters must be valid.
> > * Added RTE_ASSERT for parameters instead.
> >   Code for this is only generated if built with RTE_ENABLE_ASSERT.
> > * Removed fallback when 'cache' parameter is not set. (Honnappa)
> > * Chose the simple get function; i.e. do not move the existing
> objects in
> >   the cache to the top of the new stack, just leave them at the
> bottom.
> > * Renamed the functions. Other suggestions are welcome, of course. ;-
> )
> > * Updated the function descriptions.
> > * Added the functions to trace_fp and version.map.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---

[...]

> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: This API may change, or be removed, without
> prior
> > notice.
> > + *
> > + * Zero-copy put objects in a user-owned mempool cache backed by the
> > specified mempool.
> > + *
> > + * @param cache
> > + *   A pointer to the mempool cache.
> > + * @param mp
> > + *   A pointer to the mempool.
> > + * @param n
> > + *   The number of objects to be put in the mempool cache.
> > + *   Must not exceed RTE_MEMPOOL_CACHE_MAX_SIZE.
> > + * @return
> > + *   The pointer to where to put the objects in the mempool cache.
> > + */
> 
> rte_mempool_cache_zc_put_bulk function takes *cache as an input
> parameter, which means rte_mempool_default_cache function must be
> called in the PMD code, because there is no pointer to mempool stored
> in i40e_tx_queue. Its there in i40e_rx_queue though.
> So, should we change the API's ?

Excellent question!

This is a "mempool cache" API. So we must keep in mind that it can be consumed by applications and other libraries using mempool caches, not just PMDs.

If some consumer of the API, e.g. i40e_tx, doesn’t know the cache pointer, it must look it up itself before calling the function, e.g. by rte_mempool_default_cache().

I think we should keep the API clean, as proposed. Otherwise, the added lookup (although conditional) would degrade the performance of all other consumers of the API.

And there is no performance difference for the PMD whether it calls rte_mempool_default_cache() in the PMD itself, or if it is called from within the rte_mempool_cache_zc_put_bulk() function.

> 
> > +__rte_experimental
> > +static __rte_always_inline void *
> > +rte_mempool_cache_zc_put_bulk(struct rte_mempool_cache *cache,
> > +		struct rte_mempool *mp,
> > +		unsigned int n)
> > +{
> > +	void **cache_objs;
> > +
> > +	RTE_ASSERT(cache != NULL);
> > +	RTE_ASSERT(mp != NULL);
> > +	RTE_ASSERT(n <= RTE_MEMPOOL_CACHE_MAX_SIZE);
> > +
> > +	rte_mempool_trace_cache_zc_put_bulk(cache, mp, n);
> > +
> > +	/* Increment stats now, adding in mempool always succeeds. */
> > +	RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_bulk, 1);
> > +	RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_objs, n);
> > +
> > +	/*
> > +	 * The cache follows the following algorithm:
> > +	 *   1. If the objects cannot be added to the cache without
> crossing
> > +	 *      the flush threshold, flush the cache to the backend.
> > +	 *   2. Add the objects to the cache.
> > +	 */
> > +
> > +	if (cache->len + n <= cache->flushthresh) {
> > +		cache_objs = &cache->objs[cache->len];
> > +		cache->len += n;
> > +	} else {
> > +		cache_objs = &cache->objs[0];
> > +		rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache-
> > >len);
> > +		cache->len = n;
> > +	}
> > +
> > +	return cache_objs;
> > +}
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: This API may change, or be removed, without
> prior
> > notice.
> > + *
> > + * Zero-copy get objects from a user-owned mempool cache backed by
> the
> > specified mempool.
> > + *
> > + * @param cache
> > + *   A pointer to the mempool cache.
> > + * @param mp
> > + *   A pointer to the mempool.
> > + * @param n
> > + *   The number of objects to prefetch into the mempool cache.
> > + *   Must not exceed RTE_MEMPOOL_CACHE_MAX_SIZE.
> > + * @return
> > + *   The pointer to the objects in the mempool cache.
> > + *   NULL on error; i.e. the cache + the pool does not contain n
> objects.
> > + *   With rte_errno set to the error code of the mempool dequeue
> function.
> > + */
> > +__rte_experimental
> > +static __rte_always_inline void *
> > +rte_mempool_cache_zc_get_bulk(struct rte_mempool_cache *cache,
> > +		struct rte_mempool *mp,
> > +		unsigned int n)
> > +{
> > +	unsigned int len;
> > +
> > +	RTE_ASSERT(cache != NULL);
> > +	RTE_ASSERT(mp != NULL);
> > +	RTE_ASSERT(n <= RTE_MEMPOOL_CACHE_MAX_SIZE);
> > +
> > +	rte_mempool_trace_cache_zc_get_bulk(cache, mp, n);
> > +
> > +	len = cache->len;
> > +
> > +	if (unlikely(n > len)) {
> > +		/* Fill the cache from the backend; fetch size + requested
> -
> > len objects. */
> > +		int ret;
> > +		const unsigned int size = cache->size;
> > +
> > +		ret = rte_mempool_ops_dequeue_bulk(mp, &cache-
> > >objs[len], size + n - len);
> > +		if (unlikely(ret < 0)) {
> > +			/*
> > +			 * We are buffer constrained.
> > +			 * Do not fill the cache, just satisfy the request.
> > +			 */
> > +			ret = rte_mempool_ops_dequeue_bulk(mp, &cache-
> > >objs[len], n - len);
> > +			if (unlikely(ret < 0)) {
> > +				/* Unable to satisfy the request. */
> > +
> > +				RTE_MEMPOOL_STAT_ADD(mp,
> > get_fail_bulk, 1);
> > +				RTE_MEMPOOL_STAT_ADD(mp,
> > get_fail_objs, n);
> > +
> > +				rte_errno = -ret;
> > +				return NULL;
> > +			}
> > +
> > +			len = 0;
> > +		} else
> > +			len = size;
> > +	} else
> > +		len -= n;
> > +
> > +	cache->len = len;
> > +
> > +	RTE_MEMPOOL_CACHE_STAT_ADD(cache, get_success_bulk, 1);
> > +	RTE_MEMPOOL_CACHE_STAT_ADD(cache, get_success_objs, n);
> > +
> > +	return &cache->objs[len];
> > +}
> > +