From patchwork Wed Oct 5 09:50:31 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew Rybchenko X-Patchwork-Id: 117376 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 76D8EA0542; Wed, 5 Oct 2022 11:50:51 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 06D91410FB; Wed, 5 Oct 2022 11:50:48 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A894740694 for ; Wed, 5 Oct 2022 11:50:46 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 115) id 7CCA883; Wed, 5 Oct 2022 12:50:46 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mail1.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=ALL_TRUSTED, DKIM_ADSP_DISCARD autolearn=no autolearn_force=no version=3.4.6 Received: from aros.oktetlabs.ru (aros.oktetlabs.ru [192.168.38.17]) by shelob.oktetlabs.ru (Postfix) with ESMTP id D7AB666; Wed, 5 Oct 2022 12:50:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru D7AB666 Authentication-Results: shelob.oktetlabs.ru/D7AB666; dkim=none; dkim-atps=neutral From: Andrew Rybchenko To: Olivier Matz Cc: dev@dpdk.org, =?utf-8?q?Morten_Br=C3=B8rup?= , Beilei Xing , Bruce Richardson , Jerin Jacob Kollanukkaran Subject: [PATCH v3] mempool: fix get objects from mempool with cache Date: Wed, 5 Oct 2022 12:50:31 +0300 Message-Id: <20221005095037.997006-2-andrew.rybchenko@oktetlabs.ru> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20221005095037.997006-1-andrew.rybchenko@oktetlabs.ru> References: <20220624102354.1516606-1-ciara.loftus@intel.com> <20221005095037.997006-1-andrew.rybchenko@oktetlabs.ru> MIME-Version: 1.0 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 From: Morten Brørup A flush threshold for the mempool cache was introduced in DPDK version 1.3, but rte_mempool_do_generic_get() was not completely updated back then, and some inefficiencies were introduced. Fix the following in rte_mempool_do_generic_get(): 1. The code that initially screens the cache request was not updated with the change in DPDK version 1.3. The initial screening compared the request length to the cache size, which was correct before, but became irrelevant with the introduction of the flush threshold. E.g. the cache can hold up to flushthresh objects, which is more than its size, so some requests were not served from the cache, even though they could be. The initial screening has now been corrected to match the initial screening in rte_mempool_do_generic_put(), which verifies that a cache is present, and that the length of the request does not overflow the memory allocated for the cache. This bug caused a major performance degradation in scenarios where the application burst length is the same as the cache size. In such cases, the objects were not ever fetched from the mempool cache, regardless if they could have been. This scenario occurs e.g. if an application has configured a mempool with a size matching the application's burst size. 2. The function is a helper for rte_mempool_generic_get(), so it must behave according to the description of that function. Specifically, objects must first be returned from the cache, subsequently from the ring. After the change in DPDK version 1.3, this was not the behavior when the request was partially satisfied from the cache; instead, the objects from the ring were returned ahead of the objects from the cache. This bug degraded application performance on CPUs with a small L1 cache, which benefit from having the hot objects first in the returned array. (This is probably also the reason why the function returns the objects in reverse order, which it still does.) Now, all code paths first return objects from the cache, subsequently from the ring. The function was not behaving as described (by the function using it) and expected by applications using it. This in itself is also a bug. 3. If the cache could not be backfilled, the function would attempt to get all the requested objects from the ring (instead of only the number of requested objects minus the objects available in the ring), and the function would fail if that failed. Now, the first part of the request is always satisfied from the cache, and if the subsequent backfilling of the cache from the ring fails, only the remaining requested objects are retrieved from the ring. The function would fail despite there are enough objects in the cache plus the common pool. 4. The code flow for satisfying the request from the cache was slightly inefficient: The likely code path where the objects are simply served from the cache was treated as unlikely. Now it is treated as likely. Signed-off-by: Morten Brørup Signed-off-by: Andrew Rybchenko --- v3 changes (Andrew Rybchenko) - Always get first objects from the cache even if request is bigger than cache size. Remove one corresponding condition from the path when request is fully served from cache. - Simplify code to avoid duplication: - Get objects directly from backend in single place only. - Share code which gets from the cache first regardless if everythihg is obtained from the cache or just the first part. - Rollback cache length in unlikely failure branch to avoid cache vs NULL check in success branch. v2 changes - Do not modify description of return value. This belongs in a separate doc fix. - Elaborate even more on which bugs the modifications fix. lib/mempool/rte_mempool.h | 74 +++++++++++++++++++++++++-------------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h index a3c4ee351d..58e41ed401 100644 --- a/lib/mempool/rte_mempool.h +++ b/lib/mempool/rte_mempool.h @@ -1443,41 +1443,54 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, unsigned int n, struct rte_mempool_cache *cache) { int ret; + unsigned int remaining = n; uint32_t index, len; void **cache_objs; - /* No cache provided or cannot be satisfied from cache */ - if (unlikely(cache == NULL || n >= cache->size)) + /* No cache provided */ + if (unlikely(cache == NULL)) goto ring_dequeue; - cache_objs = cache->objs; + /* Use the cache as much as we have to return hot objects first */ + len = RTE_MIN(remaining, cache->len); + cache_objs = &cache->objs[cache->len]; + cache->len -= len; + remaining -= len; + for (index = 0; index < len; index++) + *obj_table++ = *--cache_objs; - /* Can this be satisfied from the cache? */ - if (cache->len < n) { - /* No. Backfill the cache first, and then fill from it */ - uint32_t req = n + (cache->size - cache->len); + if (remaining == 0) { + /* The entire request is satisfied from the cache. */ - /* How many do we require i.e. number to fill the cache + the request */ - ret = rte_mempool_ops_dequeue_bulk(mp, - &cache->objs[cache->len], req); - if (unlikely(ret < 0)) { - /* - * In the off chance that we are buffer constrained, - * where we are not able to allocate cache + n, go to - * the ring directly. If that fails, we are truly out of - * buffers. - */ - goto ring_dequeue; - } + RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); + RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); - cache->len += req; + return 0; } - /* Now fill in the response ... */ - for (index = 0, len = cache->len - 1; index < n; ++index, len--, obj_table++) - *obj_table = cache_objs[len]; + /* if dequeue below would overflow mem allocated for cache */ + if (unlikely(remaining > RTE_MEMPOOL_CACHE_MAX_SIZE)) + goto ring_dequeue; - cache->len -= n; + /* Fill the cache from the ring; fetch size + remaining objects. */ + ret = rte_mempool_ops_dequeue_bulk(mp, cache->objs, + cache->size + remaining); + if (unlikely(ret < 0)) { + /* + * We are buffer constrained, and not able to allocate + * cache + remaining. + * Do not fill the cache, just satisfy the remaining part of + * the request directly from the ring. + */ + goto ring_dequeue; + } + + /* Satisfy the remaining part of the request from the filled cache. */ + cache_objs = &cache->objs[cache->size + remaining]; + for (index = 0; index < remaining; index++) + *obj_table++ = *--cache_objs; + + cache->len = cache->size; RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); @@ -1486,10 +1499,19 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, ring_dequeue: - /* get remaining objects from ring */ - ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); + /* Get the objects directly from the ring. */ + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, remaining); if (ret < 0) { + if (cache != NULL) { + cache->len = n - remaining; + /* + * No further action is required to roll the first part + * of the request back into the cache, as objects in + * the cache are intact. + */ + } + RTE_MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1); RTE_MEMPOOL_STAT_ADD(mp, get_fail_objs, n); } else {