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 9DCE741D8F; Mon, 27 Feb 2023 11:39:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30E3940A7D; Mon, 27 Feb 2023 11:39:15 +0100 (CET) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by mails.dpdk.org (Postfix) with ESMTP id 36AAB400D5 for ; Mon, 27 Feb 2023 11:39:13 +0100 (CET) Received: by mail-wr1-f44.google.com with SMTP id e37so3197058wri.10 for ; Mon, 27 Feb 2023 02:39:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=kdgc7dle1D4eYJ8Oo3fp1HIR3JDie48XG2+svtGB1dc=; b=X75uIiN7lMmnUeAfPktP2e0sMH76IEZFPYXcNw8oN55DrlZANRgEMvIxVN8LA6Y1BU oyWUDo/jEtjWtkBujOH6jddslCXtlG08kP49QOI83eIJb8UbRZeNGd9z1ojYx6OrOApY EIiAO9AIBDGYsvKZHd1guFUX9l4brHrWAWh9AOg8eoKAkSHFSpnBLbL9oRqDMaEQL/re LHTbF0sZDrJBVu7lj94k9n+qNSzvkOYEKA/77bvW3XEnRufNXDIqBSP/2C+CcVmbxusY kpofgaaNon2dmaGGvJ+n4LJtSzw0RQ53Oo3zD/jH3uiLNYOnZk0w6+l489iz/cW0APv5 Wf/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=kdgc7dle1D4eYJ8Oo3fp1HIR3JDie48XG2+svtGB1dc=; b=uR10p1usf6StAYazK8ScbBlnA8dO+2i23ftXVhEoXXMrxtXcyM9txUEgfwNoEsW+Dq GP3a4VX58qaf0hRD0tFn+UN1A9oByWfXLleJzVr48I/psDvqucLjcWesoC0q6RExyTgI 7EgcVjRaAgeNCehQ/PyPXa0mdUa9htafcWO1/2v3X39t4rHjo7q1hdGwVB9oszLel+26 Xxb4IFm9G84PecBPsyOWxujuHibtkncxB/0bUgdaPUmp0i1wqjkPAPI4kQHU6E0oOEIB OvJ4nLBr/qcbUW9eT99lPoZMGC9aI2HyICIdFY9ioaeXYtvq8EeTEoC152ECB6K1yeow jboQ== X-Gm-Message-State: AO0yUKWscG+Cmaxl3QULq4aAOqodV21o+bKDqkoF7k6RJ/L3dcwKYUQN WnTzEWoZsM6+a0lyu+brXQ/OnA== X-Google-Smtp-Source: AK7set8LKQVZGuCt2yhtrvXoa9Nb++44dkzqKZomFDQWY9c8Mv4Cpf5qo922Nb9HZLAghsk3Jx3RtA== X-Received: by 2002:a5d:4847:0:b0:2c7:96ad:efc0 with SMTP id n7-20020a5d4847000000b002c796adefc0mr7231821wrs.28.1677494352831; Mon, 27 Feb 2023 02:39:12 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id q5-20020adff505000000b002c70a0e2cd0sm6850309wro.101.2023.02.27.02.39.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Feb 2023 02:39:12 -0800 (PST) Date: Mon, 27 Feb 2023 11:39:11 +0100 From: Olivier Matz To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: "Harris, James R" , Honnappa Nagarahalli , dev@dpdk.org, nd , andrew.rybchenko@oktetlabs.ru, bruce.richardson@intel.com Subject: Re: Bug in rte_mempool_do_generic_get? Message-ID: References: <98CBD80474FA8B44BF855DF32C47DC35D87770@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87774@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D8777F@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D8777F@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 Mon, Feb 27, 2023 at 10:48:09AM +0100, Morten Brørup wrote: > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > Sent: Monday, 27 February 2023 10.10 > > > > Hi, > > > > On Sun, Feb 26, 2023 at 11:45:48AM +0100, Morten Brørup wrote: > > > > From: Harris, James R [mailto:james.r.harris@intel.com] > > > > Sent: Friday, 24 February 2023 17.43 > > > > > > > > > On Feb 24, 2023, at 6:56 AM, Honnappa Nagarahalli > > > > wrote: > > > > > > > > > >> From: Morten Brørup > > > > >> Sent: Friday, February 24, 2023 6:13 AM > > > > >> To: Harris, James R ; dev@dpdk.org > > > > >> Subject: RE: Bug in rte_mempool_do_generic_get? > > > > >> > > > > >>> If you have a mempool with 2048 objects, shouldn't 4 cores each be > > > > able to do a 512 buffer bulk get, regardless of the configured cache > > > > size? > > > > >> > > > > >> No, the scenario you described above is the expected behavior. I > > > > think it is > > > > >> documented somewhere that objects in the caches are unavailable for > > > > other > > > > >> cores, but now I cannot find where this is documented. > > > > >> > > > > > > > > Thanks Morten. > > > > > > > > Yeah, I think it is documented somewhere, but I also couldn’t find it. > > > > I was aware of cores not being able to allocate from another core’s > > > > cache. My surprise was that in a pristine new mempool, that 4 cores > > > > could not each do one initial 512 buffer bulk get. But I also see that > > > > even before the a2833ecc5 patch, the cache would get populated on gets > > > > less than cache size, in addition to the buffers requested by the user. > > > > So if cache size is 256, and bulk get is for 128 buffers, it pulls 384 > > > > buffers from backing pool - 128 for the caller, another 256 to prefill > > > > the cache. Your patch makes this cache filling consistent between less- > > > > than-cache-size and greater-than-or-equal-to-cache-size cases. > > > > > > Yeah... I have tried hard to clean up some of the mess introduced by the > > patch [1] that increased the effective cache size by > > factor 1.5. > > > > > > [1] > > http://git.dpdk.org/dpdk/commit/lib/librte_mempool/rte_mempool.h?id=ea5dd2744b > > 90b330f07fd10f327ab99ef55c7266 > > > > > > E.g., somewhat related to your use case, I have argued against the design > > goal of trying to keep the mempool caches full at all times, but have not yet > > been able to convince the community against it. > > > > > > Overall, this is one of the absolute core DPDK libraries, so the support for > > the changes I would like to see is near zero - even minor adjustments are > > considered very high risk, which I must admit has quite a lot of merit. > > > > > > So instead of fixing the library's implementation to make it behave as > > reasonably expected according to its documentation, the library's > > implementation is considered the reference. And as a consequence, the > > library's internals, such as the cache size multiplier, is getting promoted to > > be part of the public API, e.g. for applications to implement mempool sizing > > calculations like the one below. > > > > > > I recall running into problems once myself, when I had sized a mempool with > > a specific cache size for a specific number of cores, because 50 % additional > > objects got stuck in the caches. If you ask me, this bug is so fundamental > > that it should be fixed at the root, not by trying to document the weird > > behavior of allocating 50 % more than specified. > > > > I think we should document it: even in the case the 1.5 factor is > > removed, it is helpful to document that a user needs to reserve more > > objects when using a cache, to ensure that all cores can allocate their > > objects. > > > > Morten, what would you think about this change below? > > A big improvement! Only a few comments, inline below. > > > > > --- a/doc/guides/prog_guide/mempool_lib.rst > > +++ b/doc/guides/prog_guide/mempool_lib.rst > > @@ -89,9 +89,13 @@ In this way, each core has full access to its own cache > > (with locks) of free obj > > only when the cache fills does the core need to shuffle some of the free > > objects back to the pools ring or > > obtain more objects when the cache is empty. > > > > -While this may mean a number of buffers may sit idle on some core's cache, > > +While this may mean a number of buffers may sit idle on some core's cache > > (up to ``1.5 * cache_length``), > > Typo: cache_length -> cache_size > > Looking at this, I noticed another detail: Chapter 10.4 uses the term "buffers", but it should use the term "objects", because a mempool may hold other objects than packet buffers. (Using the term "packet buffers" in the example in chapter 10.3 is correct.) > > > the speed at which a core can access its own cache for a specific memory > > pool without locks provides performance gains. > > > > +However, to ensure that all cores can get objects when a cache is used, it > > is required to allocate > > +more objects: if N objects are needed, allocate a mempool with ``N + > > (number_of_active_cores * > > +cache_size * 1.5)`` objects. > > + > > Perhaps add here that the factor 1.5 stems from the cache being allowed to exceed its configured size by 50 %. > > > The cache is composed of a small, per-core table of pointers and its length > > (used as a stack). > > This internal cache can be enabled or disabled at creation of the pool. > > > > If something similar is added to the description of the "cache_size" parameter to the "rte_mempool_create()" function, the most obvious places to look for documentation are covered. > > A lengthy description might be redundant here, so perhaps only mention that the actual cache size is 1.5 * cache_size, because the cache is allowed to exceed its configured size by 50 %. Thanks for the quick feedback, I'll submit a patch for this. > > > > > > > > > > > > > > >> Furthermore, since the effective per-core cache size is 1.5 * > > > > configured cache > > > > >> size, a configured cache size of 256 may leave up to 384 objects in > > > > each per- > > > > >> core cache. > > > > >> > > > > >> With 4 cores, you can expect up to 3 * 384 = 1152 objects sitting in > > > > the > > > > >> caches of other cores. If you want to be able to pull 512 objects > > > > with each > > > > >> core, the pool size should be 4 * 512 + 1152 objects. > > > > > May be we should document this in mempool library? > > > > > > > > > > > > > Maybe. But this case I described here is a bit wonky - SPDK should > > > > never have been specifying a non-zero cache in this case. We only > > > > noticed this change in behavior because we were creating the mempool > > > > with a cache when we shouldn’t have. > > > > > > So, looking at the positive side, this regression revealed a "bug" in SPDK. > > ;-) > > > > > > PS: I assume that you are aware that mempools generally perform better with > > cache, so I assume that you know what you are doing when you say that you > > don't need the cache. > > > > > > PPS: Sorry about venting here. If nothing else, I hope it had some > > entertainment value. :-) > > > >