From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Konstantin Ananyev" <konstantin.ananyev@huawei.com>,
"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
<olivier.matz@6wind.com>, <andrew.rybchenko@oktetlabs.ru>,
<honnappa.nagarahalli@arm.com>, <kamalakshitha.aligeri@arm.com>,
<bruce.richardson@intel.com>, <dev@dpdk.org>
Cc: <nd@arm.com>
Subject: RE: [PATCH v5] mempool cache: add zero-copy get and put functions
Date: Mon, 23 Jan 2023 13:23:50 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D876A9@smartserver.smartshare.dk> (raw)
In-Reply-To: <81e1ec960f6d4a1c80b236d9314cb549@huawei.com>
> From: Konstantin Ananyev [mailto:konstantin.ananyev@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.ananyev@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. */
next prev parent reply other threads:[~2023-01-23 12:23 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-05 13:19 [RFC]: mempool: zero-copy cache get bulk Morten Brørup
2022-11-07 9:19 ` Bruce Richardson
2022-11-07 14:32 ` Morten Brørup
2022-11-15 16:18 ` [PATCH] mempool cache: add zero-copy get and put functions Morten Brørup
2022-11-16 18:04 ` [PATCH v2] " Morten Brørup
2022-11-29 20:54 ` Kamalakshitha Aligeri
2022-11-30 10:21 ` Morten Brørup
2022-12-22 15:57 ` Konstantin Ananyev
2022-12-22 17:55 ` Morten Brørup
2022-12-23 16:58 ` Konstantin Ananyev
2022-12-24 12:17 ` Morten Brørup
2022-12-24 11:49 ` [PATCH v3] " Morten Brørup
2022-12-24 11:55 ` [PATCH v4] " Morten Brørup
2022-12-27 9:24 ` Andrew Rybchenko
2022-12-27 10:31 ` Morten Brørup
2022-12-27 15:17 ` [PATCH v5] " Morten Brørup
2023-01-22 20:34 ` Konstantin Ananyev
2023-01-22 21:17 ` Morten Brørup
2023-01-23 11:53 ` Konstantin Ananyev
2023-01-23 12:23 ` Morten Brørup [this message]
2023-01-23 12:52 ` Konstantin Ananyev
2023-01-23 14:30 ` Bruce Richardson
2023-01-24 1:53 ` Kamalakshitha Aligeri
2023-02-09 14:39 ` [PATCH v6] " Morten Brørup
2023-02-09 14:52 ` [PATCH v7] " Morten Brørup
2023-02-09 14:58 ` [PATCH v8] " Morten Brørup
2023-02-10 8:35 ` fengchengwen
2023-02-12 19:56 ` Honnappa Nagarahalli
2023-02-12 23:15 ` Morten Brørup
2023-02-13 4:29 ` Honnappa Nagarahalli
2023-02-13 9:30 ` Morten Brørup
2023-02-13 9:37 ` Olivier Matz
2023-02-13 10:25 ` Morten Brørup
2023-02-14 14:16 ` Andrew Rybchenko
2023-02-13 12:24 ` [PATCH v9] " Morten Brørup
2023-02-13 14:33 ` Kamalakshitha Aligeri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D876A9@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=honnappa.nagarahalli@arm.com \
--cc=kamalakshitha.aligeri@arm.com \
--cc=konstantin.ananyev@huawei.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=nd@arm.com \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).