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 93F31A050B; Thu, 7 Apr 2022 11:14:37 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 45AC14068B; Thu, 7 Apr 2022 11:14:37 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id 2AD3F40689 for ; Thu, 7 Apr 2022 11:14:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1649322875; x=1680858875; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=TUeWNIagBX9ZgAcPwDcB9knrO0hQNN7Ab1fEVAtrYl0=; b=BhH98aqd9KQ1repUbYmUNbBXR4j9eaIr7FQzMiegZin9XWJJ7bmO3+q6 head1zt26fxYPg+BFC39G8WZqjvavxT6+wUVUf3SjUVX6tB/PfFNVzSSe z2+jYvicEs6O+H+zlUcwdnw4aCE9AoYlIv5LGxvCcSz7jSdChOYAsTVsM SfNTzcUFXlN8njpz1YYfqeMiSBwCFFUw5fCKisZaEnLRqsFfSFPh20Sgk DdsZ2V+EKbVJSkJu/ammnfkD8ZIH9QGzz/6208HwItWTvckTDS1eCUcpU GdW5GhnUmb7UT2ixouyMx4l2vqlJjP0bRm+6zhtCLJkRuMOU4WIDOJZfg A==; X-IronPort-AV: E=McAfee;i="6200,9189,10309"; a="347713237" X-IronPort-AV: E=Sophos;i="5.90,241,1643702400"; d="scan'208";a="347713237" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2022 02:14:34 -0700 X-IronPort-AV: E=Sophos;i="5.90,241,1643702400"; d="scan'208";a="722885836" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.67]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 07 Apr 2022 02:14:31 -0700 Date: Thu, 7 Apr 2022 10:14:28 +0100 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: olivier.matz@6wind.com, andrew.rybchenko@oktetlabs.ru, thomas@monjalon.net, jerinjacobk@gmail.com, dev@dpdk.org Subject: Re: [PATCH v4] mempool: fix mempool cache flushing algorithm Message-ID: References: <98CBD80474FA8B44BF855DF32C47DC35D86DB2@smartserver.smartshare.dk> <20220202103354.79832-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D86FB8@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D86FB8@smartserver.smartshare.dk> 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 On Thu, Apr 07, 2022 at 11:04:53AM +0200, Morten Brørup wrote: > > From: Morten Brørup [mailto:mb@smartsharesystems.com] > > Sent: Wednesday, 2 February 2022 11.34 > > > > This patch fixes the rte_mempool_do_generic_put() caching algorithm, > > which was fundamentally wrong, causing multiple performance issues when > > flushing. > > > > [...] > > Olivier, > > Will you please consider this patch [1] and the other one [2]. > > The primary bug here is this: When a mempool cache becomes full (i.e. exceeds the "flush threshold"), and is flushed to the backing ring, it is still full afterwards; but it should be empty afterwards. It is not flushed entirely, only the elements exceeding "size" are flushed. > I don't believe it should be flushed entirely, there should always be some elements left so that even after flush we can still allocate an additional burst. We want to avoid the situation where a flush of all elements is immediately followed by a refill of new elements. However, we can flush to maybe size/2, and improve things. In short, this not emptying is by design rather than a bug, though we can look to tweak the behaviour. > E.g. pipelined applications having ingress threads and egress threads running on different lcores are affected by this bug. > If we are looking at improvements for pipelined applications, I think a bigger win would be to change the default mempool from ring-based to stack-based. For apps using a run-to-completion model, they should run out of cache and should therefore be largely unaffected by such a change. > I don't think the real performance impact is very big, but these algorithm level bugs really annoy me. > > I'm still wondering how the patch introducing the mempool cache flush threshold could pass internal code review with so many bugs. > > [1] https://patchwork.dpdk.org/project/dpdk/patch/20220202103354.79832-1-mb@smartsharesystems.com/ > [2] https://patchwork.dpdk.org/project/dpdk/patch/20220202081426.77975-1-mb@smartsharesystems.com/ > > -Morten > > > Signed-off-by: Morten Brørup > > --- > > lib/mempool/rte_mempool.h | 34 ++++++++++++++++++++++------------ > > 1 file changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > > index 1e7a3c1527..e7e09e48fc 100644 > > --- a/lib/mempool/rte_mempool.h > > +++ b/lib/mempool/rte_mempool.h > > @@ -1344,31 +1344,41 @@ rte_mempool_do_generic_put(struct rte_mempool > > *mp, void * const *obj_table, > > if (unlikely(cache == NULL || n > RTE_MEMPOOL_CACHE_MAX_SIZE)) > > goto ring_enqueue; > > > > - cache_objs = &cache->objs[cache->len]; > > + /* If the request itself is too big for the cache */ > > + if (unlikely(n > cache->flushthresh)) > > + goto ring_enqueue; > > > > /* > > * The cache follows the following algorithm > > - * 1. Add the objects to the cache > > - * 2. Anything greater than the cache min value (if it crosses > > the > > - * cache flush threshold) is flushed to the ring. > > In the code, "the cache min value" is actually "the cache size". This indicates an intention to do something more. Perhaps the patch introducing the "flush threshold" was committed while still incomplete, and just never got completed? > > > + * 1. If the objects cannot be added to the cache without > > + * crossing the flush threshold, flush the cache to the ring. > > + * 2. Add the objects to the cache. > > */ > > > > - /* Add elements back into the cache */ > > - rte_memcpy(&cache_objs[0], obj_table, sizeof(void *) * n); > > + if (cache->len + n <= cache->flushthresh) { > > + cache_objs = &cache->objs[cache->len]; > > > > - cache->len += n; > > + cache->len += n; > > + } else { > > + cache_objs = &cache->objs[0]; > > > > - if (cache->len >= cache->flushthresh) { > > - rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache->size], > > - cache->len - cache->size); > > - cache->len = cache->size; > > +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG > > + if (rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache- > > >len) < 0) > > + rte_panic("cannot put objects in mempool\n"); > > +#else > > + rte_mempool_ops_enqueue_bulk(mp, cache_objs, cache->len); > > +#endif > > + cache->len = n; > > } > > > > + /* Add the objects to the cache. */ > > + rte_memcpy(cache_objs, obj_table, sizeof(void *) * n); > > + > > return; > > > > ring_enqueue: > > > > - /* push remaining objects in ring */ > > + /* Put the objects into the ring */ > > #ifdef RTE_LIBRTE_MEMPOOL_DEBUG > > if (rte_mempool_ops_enqueue_bulk(mp, obj_table, n) < 0) > > rte_panic("cannot put objects in mempool\n"); > > -- > > 2.17.1 >