From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DBD07A0093; Mon, 7 Nov 2022 15:32:54 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7A5A740156; Mon, 7 Nov 2022 15:32:54 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 9315F40151 for ; Mon, 7 Nov 2022 15:32:52 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [RFC]: mempool: zero-copy cache get bulk Date: Mon, 7 Nov 2022 15:32:47 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87492@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [RFC]: mempool: zero-copy cache get bulk Thread-Index: AdjyigazQN5v5mWlQAqSQq8oYn5LkAAI3H7A References: <98CBD80474FA8B44BF855DF32C47DC35D87488@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Kamalakshitha Aligeri" Cc: , , , , "nd" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org + Akshitha, apparently working on similar patches > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Monday, 7 November 2022 10.19 >=20 > On Sat, Nov 05, 2022 at 02:19:13PM +0100, Morten Br=F8rup 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=3D1052 > > > > > > 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. > :-) > > >=20 > 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=3Da2833ecc5e= a4adcbc3b77e7aeac2a6fd945da6a0 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. >=20 > 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: >=20 > - rte_mempool_cache_pop? > - rte_mempool_cache_reserve? >=20 > 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. >=20 > 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@smartser= ver.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. >=20 > Overall, though, I think this is very good to have. > /Bruce