From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Kamalakshitha Aligeri" <Kamalakshitha.Aligeri@arm.com>,
<olivier.matz@6wind.com>, <andrew.rybchenko@oktetlabs.ru>,
"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>,
<bruce.richardson@intel.com>, <dev@dpdk.org>
Cc: "nd" <nd@arm.com>
Subject: RE: [PATCH v2] mempool cache: add zero-copy get and put functions
Date: Wed, 30 Nov 2022 11:21:08 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8752C@smartserver.smartshare.dk> (raw)
In-Reply-To: <DB7PR08MB386574825E5455C402E8D6B4F7129@DB7PR08MB3865.eurprd08.prod.outlook.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];
> > +}
> > +
next prev parent reply other threads:[~2022-11-30 10:21 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 [this message]
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
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=98CBD80474FA8B44BF855DF32C47DC35D8752C@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Kamalakshitha.Aligeri@arm.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--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).