From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>,
"Kamalakshitha Aligeri" <Kamalakshitha.Aligeri@arm.com>
Cc: <dev@dpdk.org>, <olivier.matz@6wind.com>,
<andrew.rybchenko@oktetlabs.ru>, <honnappa.nagarahalli@arm.com>,
"nd" <nd@arm.com>
Subject: RE: [RFC]: mempool: zero-copy cache get bulk
Date: Mon, 7 Nov 2022 15:32:47 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87492@smartserver.smartshare.dk> (raw)
In-Reply-To: <Y2jNjUM1F4jsTW43@bricha3-MOBL.ger.corp.intel.com>
+ Akshitha, apparently working on similar patches
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 7 November 2022 10.19
>
> On Sat, Nov 05, 2022 at 02:19:13PM +0100, Morten Brørup wrote:
> > Zero-copy access to the mempool cache 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
> >
> >
> > This RFC offers two conceptual variants of zero-copy get:
> > 1. A simple version.
> > 2. A version where existing (hot) objects in the cache are moved to
> the top of the cache before new objects from the backend driver are
> pulled in.
> >
> > I would like some early feedback. Also, which variant do you prefer?
> >
> > Notes:
> > * Allowing the 'cache' parameter to be NULL, and getting it from the
> mempool instead, was inspired by rte_mempool_cache_flush().
> > * Asserting that the 'mp' parameter is not NULL is not done by other
> functions, so I omitted it here too.
> >
> > NB: Please ignore formatting. Also, this code has not even been
> compile tested.
> >
> >
> > PS: No promises, but I expect to offer an RFC for zero-copy put too.
> :-)
> >
>
> Thanks for this work, I think it's good to have. The existing functions
> could probably be reworked to use this new code too, right, since the
> copy
> at the end would be all that is needed to complete the implementation?
Only for the likely case, where the request can be fulfilled entirely from the cache.
Not for the corner case, where only some of the objects are in the cache, so the cache needs to be refilled from the backing store.
E.g. requesting 32 objects, and 8 objects are in the cache. (Those 8 object are assumed to be hot, as opposed to the cold objects pulled in from the backing store, and were given preferential treatment with commit [a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0].)
[a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0]: http://git.dpdk.org/dpdk/commit/lib/mempool/rte_mempool.h?id=a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0
The existing function copies the 8 existing objects directly to the final destination, then refills the cache from the backing store, and then copies the remaining 24 objects directly to the final destination.
The "2. variant" in this RFC handles this corner case by moving the 8 objects in the cache to the new top of the cache, and then refilling the cache from the backing store. And it can only move those 8 objects around in the cache if there is room for them. (The 32 returned objects are, ordered from top to bottom of the stack: 8 hot and 24 new.)
On other words: If we replaced the existing function with this function plus copying at the end, the corner case will perform additional copying (moving the objects around in the stack), whereas the existing function only copies each object once.
While I usually agree 100 % about avoiding code duplication, I think the difference in behavior between the existing and the new functions warrants two separate implementations.
Please also note: The cache is a stack, so when accessing the cache directly, objects should be retrieved in reverse order. (This should be mentioned in the function description!) The existing function reverses the order of the objects when returning them, so the application can use them in normal order.
>
> Only real comment I have on this version is that I am not sure about
> the
> naming. I think having "cache_get_bulk" doesn't really make it very
> clear
> what the function does, that is removes items from the cache without
> copying them elsewhere. How about:
>
> - rte_mempool_cache_pop?
> - rte_mempool_cache_reserve?
>
> I would tend to prefer the former, since the latter implies that there
> needs to be a follow-up call to clear the reservation. On the other
> hand,
> reserve does give the correct impression that the elements are still
> there
> in the mempool cache.
>
> Others may have better suggestions, since, as we know, naming things is
> hard! :)
- rte_mempool_cache_prefetch_bulk?
- rte_mempool_cache_get_bulk_promise?
When I came up with the function name rte_mempool_cache_put_bulk_promise for the sister function [1], I thought along the same lines as you, Bruce. It is important that the function name doesn't imply that there is a follow-up function to indicate that the transaction has been completed. (Before working on that, I assumed that a "prepare" and "commit" pair of functions were required, but the function turned out to be much simpler than anticipated.)
[1]: http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87489@smartserver.smartshare.dk/#t
The mempool library offers single-object functions, so _bulk should be part of the function name, to indicate that the function operates on more than one object.
>
> Overall, though, I think this is very good to have.
> /Bruce
next prev parent reply other threads:[~2022-11-07 14:32 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-05 13:19 Morten Brørup
2022-11-07 9:19 ` Bruce Richardson
2022-11-07 14:32 ` Morten Brørup [this message]
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
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
2022-11-06 7:12 [RFC]: mempool: zero-copy cache get bulk Morten Brørup
2022-11-13 18:31 ` Honnappa Nagarahalli
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=98CBD80474FA8B44BF855DF32C47DC35D87492@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=Kamalakshitha.Aligeri@arm.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=honnappa.nagarahalli@arm.com \
--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).