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 93C5E41D8F; Mon, 27 Feb 2023 10:09:38 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 21DA040A84; Mon, 27 Feb 2023 10:09:38 +0100 (CET) Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) by mails.dpdk.org (Postfix) with ESMTP id 8B74240A7D for ; Mon, 27 Feb 2023 10:09:36 +0100 (CET) Received: by mail-wr1-f47.google.com with SMTP id l1so2328028wry.12 for ; Mon, 27 Feb 2023 01:09:36 -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=FBvvIyuzx6oLw1tQ++eDCoYGX3/XhXYI7QBTq1pwsAA=; b=S8Akb8SZsoMr/wu4qOK0n9dKz6//NqCC+5cjxMwqG2TKGZXZj+HHpPQH9xULfionP3 HI1atoAPJgrwM3kM3pwXc0ljdjv81FELGNtUlnZMJVBl2hj3sfxKSQBfdEmkzwxk7X0h KV6iR743r3TY1kI77OSMK8FNs88dzSQTYZi9j2yGdgN7yrApMNuTrKBGgLouabO+ReKH BAhpP81Pgw3sa1Q6MkPkbfT7bMQTJgOhf3zEUMcoEv/aQcjNNfWko6HmWXdBrUF4BOI7 WZaUeHp0dRK0F7y9cAp0/ZF7wzjVJoOGfLGPryt9FXVNJNYDm015X1IZHav629BMikbp qw1w== 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=FBvvIyuzx6oLw1tQ++eDCoYGX3/XhXYI7QBTq1pwsAA=; b=Kvk5X7eKfkdjE8Dx91a0mtukwvjiNos+NO1FApkjOWLSoGm/IzFznuviqiASmSciFb gXb/SYy0hB8kC7rzO3mom5Qv0oYVt8x0E/yJ0CrDyhTV/lBI2m1OWZWOFLOoozHA6FIS 2MgU5iPflzzNU6OIsHwXuXHpF8CnIR3tkhsa2tSb5Rd8ckfiDjXop4s8H0l3V1op4pvi enHuNBURM0HyS9x3aKUVD2xShK4K7g9eMk1JliXkEouIFiwM3TbJz+sHdTRRV0Rmf/8M vjuJ+yythIytoT99hYuDuSQjhRJXulAQ7nY+lKdV+b3pHMObl2Ow2NWocjaWplRdoy/i 3i9A== X-Gm-Message-State: AO0yUKXGyvZajn9fhIEA17nyjzeKwvfmtv7oj6m/+dKtAUVZ5KwnF7YP dhTDXSFngbt3KRO0luGERu+xbg== X-Google-Smtp-Source: AK7set9ywyQV1SO72XjU6gaJmzYT0PsS7uFminjT+TD3TGEVfnic3WqfekV/uwflOfL8DaJAEyH3ew== X-Received: by 2002:a5d:4487:0:b0:2c7:cdf:e548 with SMTP id j7-20020a5d4487000000b002c70cdfe548mr13837763wrq.71.1677488976102; Mon, 27 Feb 2023 01:09:36 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id b10-20020adfee8a000000b002c54c8e70b1sm6757634wro.9.2023.02.27.01.09.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Feb 2023 01:09:35 -0800 (PST) Date: Mon, 27 Feb 2023 10:09:35 +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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87774@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 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=ea5dd2744b90b330f07fd10f327ab99ef55c7266 > > 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/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``), 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. + 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. > > > > > >> 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. :-) >