+ Akshitha, apparently working on similar patches > From: Bruce Richardson [mailto:bruce.richard...@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/98cbd80474fa8b44bf855df32c47dc35d87...@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
