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 B0354A04A8; Mon, 24 Jan 2022 16:38:59 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 466E941152; Mon, 24 Jan 2022 16:38:59 +0100 (CET) Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by mails.dpdk.org (Postfix) with ESMTP id 5AF4540040 for ; Mon, 24 Jan 2022 16:38:58 +0100 (CET) Received: by mail-wm1-f48.google.com with SMTP id f202-20020a1c1fd3000000b0034dd403f4fbso141262wmf.1 for ; Mon, 24 Jan 2022 07:38:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=wMzLEK83Wv7uqO3qnNLmrZP5dBzO+zJjecmRXKvYVoQ=; b=Q7FKDm1b0frA41TyoKd2xi2lB/lMYoV7JiRClauz0mAE80CRlymcSDxnvbzrv9NNTU uCwb+IHjg1bSbDkTm4fBCPcgeSoyd9imQfwllPNbXTuX2CBISbjVzE7iDNs4XISMEiKt GTFhwoDZxF8VX2HCZ1bo71OuEeW7lirbQdJKkPOSgcBm8TdaDln/9tNU8j1Wi0w0CyTc sOJVrVrYwWZExkuZX4UjOmzzuZuYoRjCwYW2Mxoiyw4TEA4DyF6TKYZoawYUNg0HvpQs fUSDVTeAhhZ/ig0gt/whq3B9TSgxcpRNzB+oMwAgbpV17InqGZSXJBS3ebCoVtRhAZG1 aP9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=wMzLEK83Wv7uqO3qnNLmrZP5dBzO+zJjecmRXKvYVoQ=; b=ritxObE/ssZi7NIgUbTGjfMEJf6MbwuUqCI4D5zc+I3Hc5zOF8se8ZMRcntQazY82Q dy/RsJCNnGw3aMTGFSCj2/6+VVNBxck19yA/mN9FDGxlJvyVLHjMka/e27y/LcBuTB3V /fVqE4SgEFV1KZRh1aVXITrtO/w9J3RYITS/ezI5O2Fqu4KW3movJjAg633+iZGJZK9q d+iAMLyM5xeDp3KU9lSWn+nQAt8HUOwfFHIaasBOqxqdTmdVXRDmrch7pUaZKW+4IMxZ vuo3zujJvn6lwDWTz8TcqjZPEyPgz242dlPw6ue6LIe+aEPnhKk9FNcJ+WJ9iIcmw1nF pagw== X-Gm-Message-State: AOAM531z8R6jf3f9n7KyfWXnP0FOlfvdh0gh4RYTcLrwjh+4MsiWpHwI qG0B9RXY5hREe6A87qeDGmhKrg== X-Google-Smtp-Source: ABdhPJzoMq3wdEId9zHl+YqiVLnDtg83O86/QXSwRLFku2fWs2+6ucQudq/nnDzhCYfOIWlEgpjhww== X-Received: by 2002:a05:600c:4f84:: with SMTP id n4mr2264229wmq.129.1643038738022; Mon, 24 Jan 2022 07:38:58 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id z5sm24654185wmp.10.2022.01.24.07.38.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Jan 2022 07:38:57 -0800 (PST) Date: Mon, 24 Jan 2022 16:38:56 +0100 From: Olivier Matz To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: andrew.rybchenko@oktetlabs.ru, bruce.richardson@intel.com, jerinjacobk@gmail.com, dev@dpdk.org Subject: Re: [PATCH] mempool: fix get objects from mempool with cache Message-ID: References: <98CBD80474FA8B44BF855DF32C47DC35D86DB2@smartserver.smartshare.dk> <20220114163650.94288-1-mb@smartsharesystems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220114163650.94288-1-mb@smartsharesystems.com> 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 Hi Morten, Few comments below. On Fri, Jan 14, 2022 at 05:36:50PM +0100, Morten Brørup wrote: > 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. > > This patch fixes 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. > > 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 is > bad for CPUs with a small L1 cache, which benefit from having the hot > objects first in the returned array. (This is also the reason why > the function returns the objects in reverse order.) > Now, all code paths first return objects from the cache, subsequently > from the ring. > > 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. This is the only point I'd consider to be a fix. The problem, from the user perspective, is that a get() can fail despite there are enough objects in cache + common pool. To be honnest, I feel a bit uncomfortable to have such a list of problems solved in one commit, even if I understand that they are part of the same code rework. Ideally, this fix should be a separate commit. What do you think of having this simple patch for this fix, and then do the optimizations/rework in another commit? --- a/lib/mempool/rte_mempool.h +++ b/lib/mempool/rte_mempool.h @@ -1484,7 +1484,22 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, * the ring directly. If that fails, we are truly out of * buffers. */ - goto ring_dequeue; + req = n - cache->len; + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, req); + if (ret < 0) { + RTE_MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1); + RTE_MEMPOOL_STAT_ADD(mp, get_fail_objs, n); + return ret; + } + obj_table += req; + len = cache->len; + while (len > 0) + *obj_table++ = cache_objs[--len]; + cache->len = 0; + RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); + RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); + + return 0; } cache->len += req; The title of this commit could then be more precise to describe the solved issue. > 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. > And in the code path where the cache was backfilled first, numbers were > added and subtracted from the cache length; now this code path simply > sets the cache length to its final value. > > 5. Some comments were not correct anymore. > The comments have been updated. > Most importanly, the description of the succesful return value was > inaccurate. Success only returns 0, not >= 0. > > Signed-off-by: Morten Brørup > --- > lib/mempool/rte_mempool.h | 81 ++++++++++++++++++++++++++++----------- > 1 file changed, 59 insertions(+), 22 deletions(-) > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > index 1e7a3c1527..88f1b8b7ab 100644 > --- a/lib/mempool/rte_mempool.h > +++ b/lib/mempool/rte_mempool.h > @@ -1443,6 +1443,10 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) > > /** > * @internal Get several objects from the mempool; used internally. > + * > + * If cache is enabled, objects are returned from the cache in Last In First > + * Out (LIFO) order for the benefit of CPUs with small L1 cache. > + * > * @param mp > * A pointer to the mempool structure. > * @param obj_table > @@ -1452,7 +1456,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) > * @param cache > * A pointer to a mempool cache structure. May be NULL if not needed. > * @return > - * - >=0: Success; number of objects supplied. > + * - 0: Success; got n objects. > * - <0: Error; code of ring dequeue function. > */ > static __rte_always_inline int I think that part should be in a separate commit too. This is a documentation fix, which is easily backportable (and should be backported) (Fixes: af75078fece3 ("first public release")). > @@ -1463,38 +1467,71 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, > 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 or if get would overflow mem allocated for cache */ > + if (unlikely(cache == NULL || n > RTE_MEMPOOL_CACHE_MAX_SIZE)) > goto ring_dequeue; > > - cache_objs = cache->objs; > + cache_objs = &cache->objs[cache->len]; > + > + if (n <= cache->len) { > + /* The entire request can be satisfied from the cache. */ > + cache->len -= n; > + for (index = 0; index < n; 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); > + 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); > + return 0; > + } > + > + /* Satisfy the first part of the request by depleting the cache. */ > + len = cache->len; > + for (index = 0; index < len; index++) > + *obj_table++ = *--cache_objs; > + > + /* Number of objects remaining to satisfy the request. */ > + len = n - len; > + > + /* Fill the cache from the ring; fetch size + remaining objects. */ > + ret = rte_mempool_ops_dequeue_bulk(mp, cache->objs, > + cache->size + len); > + 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. > + */ > + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, len); > 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. > + * That also failed. > + * No furter action is required to roll the first > + * part of the request back into the cache, as both > + * cache->len and the objects in the cache are intact. > */ > - goto ring_dequeue; > + RTE_MEMPOOL_STAT_ADD(mp, get_fail_bulk, 1); > + RTE_MEMPOOL_STAT_ADD(mp, get_fail_objs, n); > + > + return ret; > } > > - cache->len += req; > + /* Commit that the cache was emptied. */ > + cache->len = 0; > + > + RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); > + RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); > + > + 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]; > + cache_objs = &cache->objs[cache->size + len]; > > - cache->len -= n; > + /* Satisfy the remaining part of the request from the filled cache. */ > + cache->len = cache->size; > + for (index = 0; index < len; index++) > + *obj_table++ = *--cache_objs; > > RTE_MEMPOOL_STAT_ADD(mp, get_success_bulk, 1); > RTE_MEMPOOL_STAT_ADD(mp, get_success_objs, n); > @@ -1503,7 +1540,7 @@ rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table, > > ring_dequeue: > > - /* get remaining objects from ring */ > + /* Get the objects from the ring. */ > ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n); > > if (ret < 0) { About the code itself, it is more readable now, and probably more efficient. Did you notice any performance change in mempool perf autotests ? Thanks, Olivier