> From: Konstantin Ananyev [mailto:konstantin.anan...@huawei.com]
> Sent: Monday, 23 January 2023 12.54
>
> > > Few nits, see below.
> > > Also I still think we do need a test case for _zc_get_ before
> > > accepting it in the mainline.
> >
> > Poking at my bad conscience... :-)
> >
> > It's on my todo-list. Apparently not high enough. ;-)
> >
> > > With that in place:
> > > Acked-by: Konstantin Ananyev <konstantin.v.anan...@yandex.ru>
> > >
[...]
> > > > +/**
> > > > + * @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.
> > > > + * @return
> > > > + * The pointer to where to put the objects in the mempool
> cache.
> > > > + * NULL if the request itself is too big for the cache, i.e.
> > > > + * exceeds the cache flush threshold.
> > > > + */
> > > > +__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)
> > > > +{
> > > > + RTE_ASSERT(cache != NULL);
> > > > + RTE_ASSERT(mp != NULL);
> > > > +
> > > > + rte_mempool_trace_cache_zc_put_bulk(cache, mp, n);
> > > > + return __rte_mempool_cache_zc_put_bulk(cache, mp, n);
> > > > +}
> > > > +
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: This API may change, or be removed, without
> > > prior notice.
> > > > + *
> > > > + * Zero-copy un-put objects in a user-owned mempool cache.
> > > > + *
> > > > + * @param cache
> > > > + * A pointer to the mempool cache.
> > > > + * @param n
> > > > + * The number of objects not put in the mempool cache after
> > > calling
> > > > + * rte_mempool_cache_zc_put_bulk().
> > > > + */
> > > > +__rte_experimental
> > > > +static __rte_always_inline void
> > > > +rte_mempool_cache_zc_put_rewind(struct rte_mempool_cache *cache,
> > > > + unsigned int n)
> > > > +{
> > > > + RTE_ASSERT(cache != NULL);
> > > > + RTE_ASSERT(n <= cache->len);
> > > > +
> > > > + rte_mempool_trace_cache_zc_put_rewind(cache, n);
> > > > +
> > > > + cache->len -= n;
> > > > +
> > > > + RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_objs, (int)-n);
> > > > +}
> > > > +
> > > > +/**
> > > > + * @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.
> > >
> > > Why not 'get' instead of 'prefetch'?
> >
> > This was my thinking:
> >
> > The function "prefetches" the objects into the cache. It is the
> application itself that "gets" the objects from the cache after having
> > called the function.
> > You might also notice that the n parameter for the zc_put() function
> is described as "to be put" (future), not "to put" (now) in the
> > cache.
> >
> > On the other hand, I chose "Zero-copy get" for the function headline
> to keep it simple.
> >
> > If you think "get" is a more correct description of the n parameter,
> I can change it.
> >
> > Alternatively, I can use the same style as zc_put(), i.e. "to be
> gotten from the mempool cache" - but that would require input from a
> > natively English speaking person, because Danish and English grammar
> is very different, and I am highly uncertain about my English
> > grammar here! I originally considered this phrase, but concluded that
> the "prefetch" description was easier to understand - especially
> > for non-native English readers.
>
> For me 'prefetch' seems a bit unclear in that situation...
> Probably: "number of objects that user plans to extract from the
> cache"?
> But again, I am not native English speaker too, so might be someone can
> suggest a better option.
>
@Bruce (or any other native English speaking person), your input would be
appreciated here!
> > > > + * @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,
> > > > + * or EINVAL if the request itself is too big for the cache,
> i.e.
> > > > + * exceeds the cache flush threshold.
> > > > + */
> > > > +__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)
[...]
> > > > @@ -1364,32 +1556,25 @@ rte_mempool_do_generic_put(struct
> rte_mempool
> > > *mp, void * const *obj_table,
> > > > {
> > > > void **cache_objs;
> > > >
> > > > - /* No cache provided */
> > > > - if (unlikely(cache == NULL))
> > > > - goto driver_enqueue;
> > > > + /* No cache provided? */
> > > > + if (unlikely(cache == NULL)) {
> > > > + /* Increment stats now, adding in mempool always
> succeeds.
> > > */
> > > > + RTE_MEMPOOL_STAT_ADD(mp, put_bulk, 1);
> > > > + RTE_MEMPOOL_STAT_ADD(mp, put_objs, n);
> > > >
> > > > - /* increment stat now, adding in mempool always success */
> > > > - RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_bulk, 1);
> > > > - RTE_MEMPOOL_CACHE_STAT_ADD(cache, put_objs, n);
> > > > + goto driver_enqueue;
> > > > + }
> > > >
> > > > - /* The request itself is too big for the cache */
> > > > - if (unlikely(n > cache->flushthresh))
> > > > - goto driver_enqueue_stats_incremented;
> > > > + /* Prepare to add the objects to the cache. */
> > > > + cache_objs = __rte_mempool_cache_zc_put_bulk(cache, mp, 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.
> > > > - */
> > > > + /* The request itself is too big for the cache? */
> > > > + if (unlikely(cache_objs == NULL)) {
> > > > + /* 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);
> > >
> > > Shouldn't it be RTE_MEMPOOL_STAT_ADD() here?
> >
> > I can see why you are wondering, but the answer is no. The statistics
> in mempool cache are not related to the cache, they are related
> > to the mempool; they are there to provide faster per-lcore update
> access [1].
> >
> > [1]:
> https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/mempool/rte_mempool
> .h#L94
>
> But the condition above:
> if (unlikely(cache_objs == NULL))
> means that me can't put these object to the cache and have to put
> objects straight to the pool (skipping cache completely), right?
Correct.
> If so, then why to update cache stats instead of pool stats?
Because updating the stats in the cache structure is faster than updating the
stats in the pool structure. Refer to the two macros: RTE_MEMPOOL_STAT_ADD()
[2] is effectively five lines of code, but RTE_MEMPOOL_CACHE_STAT_ADD(cache,
name, n) [3] is a one-liner: ((cache)->stats.name += (n)).
[2]:
https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/mempool/rte_mempool.h#L325
[3]:
https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/mempool/rte_mempool.h#L348
And to reiterate that this is the correct behavior here, I will rephrase my
previous response: The stats kept in the cache are part of the pool stats, they
are not stats for the cache itself.
> > > >
> > > > - 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;
> > > > + goto driver_enqueue;
> > > > }
> > > >
> > > > /* Add the objects to the cache. */
