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 C6EDBA00C2 for ; Thu, 3 Nov 2022 10:33:19 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C3FED427F1; Thu, 3 Nov 2022 10:33:19 +0100 (CET) Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by mails.dpdk.org (Postfix) with ESMTP id CB13040693 for ; Thu, 3 Nov 2022 10:33:18 +0100 (CET) Received: by mail-wm1-f45.google.com with SMTP id i5-20020a1c3b05000000b003cf47dcd316so2859261wma.4 for ; Thu, 03 Nov 2022 02:33:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=g5lZoIRjucsogMWDezOPhqFXfjizY9L1rPwD2854ORs=; b=EObRmnx+x1UpHgnjf5WvHbr1+uBNwc7m0S/zH7gYxWllnNddgibQxW22LrPVecMaP3 vgbDKyk0USub1zI254TWkTzNl+Rvj9dZfG7ahEc7etuBTf9JYsDSkfUb3y+IixF9hFp7 1k+EUcKoo+Kk6b5SXzJlqDlBDdF5CJy26/A8WNM84xIry0EVKGCQiTHa/N1QHzjrt0Ht yoaHnjzE120zX2nHtHLrkqY3NDg+KZfsMmKx1TwPfb0paeZtIzva7mgRzwf8A1UDTWSD BuA/OhKw+B7CXnhR25KY8xMqWESp9fDl7fCskECPiHv0fyz1ujjpa6m45ioeKazH88V1 yi4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=g5lZoIRjucsogMWDezOPhqFXfjizY9L1rPwD2854ORs=; b=bkHqJLtE8ke8ZIHGrescgkjUioDNsIq76FQiSoL8QZW4dSsCpl3ey/V2lQP+zQkmhP gz+vKRaedPZfsLKfsWPo0tk+MPokCfUmOc5U1pBa/LikVW/SSasZuDMySlss9odqevjG L3TLtcTbkcpUYNNDdEG9/E5rnjvWS2hZTV9e57l9MTtCk7xgOfIaq6syk7nrV6gPoZTN ml0rfkpigJpK62dyPkF1UsQtFELf6ZwZ8P16ZuyodaHfikLHGzBLhlBuhvsNJfrF7VSf HSPwCwVsm4rtKI7XQ+98u8VI9wCrFdY2eK45CU/Gf7aX/FtyE+k3K80hWwjW7LNWzDqT LsrA== X-Gm-Message-State: ACrzQf0r6g0u8KyJdIOkRd8wGljrCpo+FOvkke0CmB+mpVgYH1tm+TfQ yx9+F/fxMWTpauooVpemXBk= X-Google-Smtp-Source: AMsMyM6v7CC+hAa43A20spJfT5ULjjbaJ1r6waacHtSInbtK1aOqhCGGMQdj5W2NVh0nS6DHuwZyng== X-Received: by 2002:a05:600c:896:b0:3cf:8e70:f341 with SMTP id l22-20020a05600c089600b003cf8e70f341mr459675wmp.74.1667467998361; Thu, 03 Nov 2022 02:33:18 -0700 (PDT) Received: from localhost ([137.220.119.58]) by smtp.gmail.com with ESMTPSA id p22-20020a05600c359600b003c6b9749505sm5027605wmq.30.2022.11.03.02.33.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Nov 2022 02:33:17 -0700 (PDT) From: luca.boccassi@gmail.com To: =?UTF-8?q?Morten=20Br=C3=B8rup?= Cc: Andrew Rybchenko , dpdk stable Subject: patch 'mempool: fix get objects from mempool with cache' has been queued to stable release 20.11.7 Date: Thu, 3 Nov 2022 09:27:53 +0000 Message-Id: <20221103092758.1099402-95-luca.boccassi@gmail.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20221103092758.1099402-1-luca.boccassi@gmail.com> References: <20221103092758.1099402-1-luca.boccassi@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Hi, FYI, your patch has been queued to stable release 20.11.7 Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. It will be pushed if I get no objections before 11/05/22. So please shout if anyone has objections. Also note that after the patch there's a diff of the upstream commit vs the patch applied to the branch. This will indicate if there was any rebasing needed to apply to the stable branch. If there were code changes for rebasing (ie: not only metadata diffs), please double check that the rebase was correctly done. Queued patches are on a temporary branch at: https://github.com/kevintraynor/dpdk-stable This queued commit can be viewed at: https://github.com/kevintraynor/dpdk-stable/commit/26cb4c81b552594292f7c744afb904f01700dfe8 Thanks. Luca Boccassi --- >From 26cb4c81b552594292f7c744afb904f01700dfe8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Morten=20Br=C3=B8rup?= Date: Fri, 7 Oct 2022 13:44:50 +0300 Subject: [PATCH] mempool: fix get objects from mempool with cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [ upstream commit a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0 ] 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 backend. 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 backend 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 backend. 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 backend (instead of only the number of requested objects minus the objects available in the backend), 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 backend fails, only the remaining requested objects are retrieved from the backend. 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 Reviewed-by: Morten Brørup --- lib/librte_mempool/rte_mempool.h | 98 +++++++++++++++++++------------- 1 file changed, 60 insertions(+), 38 deletions(-) diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h index 7516d90824..32032c53e1 100644 --- a/lib/librte_mempool/rte_mempool.h +++ b/lib/librte_mempool/rte_mempool.h @@ -1403,61 +1403,83 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) * A pointer to a mempool cache structure. May be NULL if not needed. * @return * - >=0: Success; number of objects supplied. - * - <0: Error; code of ring dequeue function. + * - <0: Error; code of driver dequeue function. */ static __rte_always_inline int __mempool_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)) - goto ring_dequeue; + /* No cache provided */ + if (unlikely(cache == NULL)) + goto driver_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)) { + __MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); + __MEMPOOL_STAT_ADD(mp, get_success_objs, n); + + return 0; + } + + /* if dequeue below would overflow mem allocated for cache */ + if (unlikely(remaining > RTE_MEMPOOL_CACHE_MAX_SIZE)) + goto driver_dequeue; + + /* Fill the cache from the backend; 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 backend. + */ + goto driver_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; + + __MEMPOOL_STAT_ADD(mp, get_success, n); + + return 0; + +driver_dequeue: + + /* Get remaining objects directly from the backend. */ + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, remaining); + + if (ret < 0) { + if (likely(cache != NULL)) { + cache->len = n - remaining; /* - * 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. + * No further action is required to roll the first part + * of the request back into the cache, as objects in + * the cache are intact. */ - goto ring_dequeue; } - cache->len += req; - } - - /* Now fill in the response ... */ - for (index = 0, len = cache->len - 1; index < n; ++index, len--, obj_table++) - *obj_table = cache_objs[len]; - - cache->len -= n; - - __MEMPOOL_STAT_ADD(mp, get_success, n); - - return 0; - -ring_dequeue: - - /* get remaining objects from ring */ - ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); - - if (ret < 0) __MEMPOOL_STAT_ADD(mp, get_fail, n); - else + } else __MEMPOOL_STAT_ADD(mp, get_success, n); return ret; -- 2.34.1 --- Diff of the applied patch vs upstream commit (please double-check if non-empty: --- --- - 2022-11-03 09:27:31.305995971 +0000 +++ 0095-mempool-fix-get-objects-from-mempool-with-cache.patch 2022-11-03 09:27:25.565426157 +0000 @@ -1 +1 @@ -From a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0 Mon Sep 17 00:00:00 2001 +From 26cb4c81b552594292f7c744afb904f01700dfe8 Mon Sep 17 00:00:00 2001 @@ -8,0 +9,2 @@ +[ upstream commit a2833ecc5ea4adcbc3b77e7aeac2a6fd945da6a0 ] + @@ -71,2 +73,2 @@ - lib/mempool/rte_mempool.h | 88 ++++++++++++++++++++++++--------------- - 1 file changed, 55 insertions(+), 33 deletions(-) + lib/librte_mempool/rte_mempool.h | 98 +++++++++++++++++++------------- + 1 file changed, 60 insertions(+), 38 deletions(-) @@ -74,5 +76,5 @@ -diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h -index 4c4af2a8ed..2401c4ac80 100644 ---- a/lib/mempool/rte_mempool.h -+++ b/lib/mempool/rte_mempool.h -@@ -1435,60 +1435,82 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) +diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h +index 7516d90824..32032c53e1 100644 +--- a/lib/librte_mempool/rte_mempool.h ++++ b/lib/librte_mempool/rte_mempool.h +@@ -1403,61 +1403,83 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) @@ -86,2 +88,2 @@ - rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, - unsigned int n, struct rte_mempool_cache *cache) + __mempool_generic_get(struct rte_mempool *mp, void **obj_table, + unsigned int n, struct rte_mempool_cache *cache) @@ -97,22 +98,0 @@ -- -- cache_objs = 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); -- -- /* 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; -- } -- -- cache->len += req; @@ -122 +102,2 @@ -+ + +- cache_objs = cache->objs; @@ -130 +111,5 @@ -+ + +- /* 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); @@ -133,3 +118,7 @@ -+ -+ RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); -+ RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); + +- /* 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)) { ++ __MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); ++ __MEMPOOL_STAT_ADD(mp, get_success_objs, n); @@ -155,5 +144,2 @@ - } - -- /* Now fill in the response ... */ -- for (index = 0, len = cache->len - 1; index < n; ++index, len--, obj_table++) -- *obj_table = cache_objs[len]; ++ } ++ @@ -164,2 +150 @@ - -- cache->len -= n; ++ @@ -167,7 +152,5 @@ - - RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); - RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); - - return 0; - --ring_dequeue: ++ ++ __MEMPOOL_STAT_ADD(mp, get_success, n); ++ ++ return 0; ++ @@ -175,3 +158 @@ - -- /* get remaining objects from ring */ -- ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); ++ @@ -180,2 +161,2 @@ - - if (ret < 0) { ++ ++ if (ret < 0) { @@ -184 +165,5 @@ -+ /* + /* +- * 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. @@ -188,6 +173,29 @@ -+ */ -+ } -+ - RTE_MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1); - RTE_MEMPOOL_STAT_ADD(mp, get_fail_objs, n); - } else { + */ +- goto ring_dequeue; + } + +- cache->len += req; +- } +- +- /* Now fill in the response ... */ +- for (index = 0, len = cache->len - 1; index < n; ++index, len--, obj_table++) +- *obj_table = cache_objs[len]; +- +- cache->len -= n; +- +- __MEMPOOL_STAT_ADD(mp, get_success, n); +- +- return 0; +- +-ring_dequeue: +- +- /* get remaining objects from ring */ +- ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); +- +- if (ret < 0) + __MEMPOOL_STAT_ADD(mp, get_fail, n); +- else ++ } else + __MEMPOOL_STAT_ADD(mp, get_success, n); + + return ret;