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 733D7A0560; Tue, 18 Oct 2022 18:33:19 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 585264069C; Tue, 18 Oct 2022 18:33:19 +0200 (CEST) Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by mails.dpdk.org (Postfix) with ESMTP id BC41340395 for ; Tue, 18 Oct 2022 18:33:17 +0200 (CEST) Received: by mail-qt1-f182.google.com with SMTP id h24so9659583qta.7 for ; Tue, 18 Oct 2022 09:33:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=u6Oe7LwSU+9rdEkeMV318uxt8R/lbVkXSGdD4rTxo1c=; b=Q5XFDFroqpZS5vL15nr5g6fB6hgUQ1QTXiBRHdn6skM6248PU0sgObuvpNDyZzcPW3 5ngF3CgxX0uAClRm4o37VqNpvd9SfNCkpX9g++gfy+mBe/BdmKBvJEgXvi68+5qEqFDo FiyGGc4do7ljnSrifORkb+AugcYaZCpXa4d4naifMCZnvuf/4mnUXBr52udGeEs6YQtr nEBPXz3vI2nK/TWiuTXJT+ZPu5TC/jM3GIsRWf7vj6VXvNpfJEKrMHgE3e0F1l5X4UsY pk4V39ddC0zZNfvWsEvhds5ciBVnfRgb6reRlBVsp2ViHuXyg9yDCDxSOrppAnmfzVD5 hpeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=u6Oe7LwSU+9rdEkeMV318uxt8R/lbVkXSGdD4rTxo1c=; b=d8S6mv8/xAVSg63kC4u7Gm1eT0wejasis5zXZ9ZfvbPYfUlzdrKPsxlz8ZbYeFkZ/H UjkckwQJRpzSYu7cgqooDtU+n5qUoArIIaE5D2ne0klfymtX2Yai008UFZ9YT5Ol/UEv B67ThFgQlhH3KcnSKT7c6c3ymSjNsBgiBhhrUrsLXW3s+ac8WO4HjH8pWdY07vceBmUm VTlM/gKLn8i5uX1x8fNyP5GRgDz6i/tBtUl0xeLxtqXW4Pxtb3pBD2QCKP8O2J7tBLKK vDxDFElcmOZ+51VC3XvtHooNYURi99QlmSemKtAhL0mSgSxnKac+xI5JYP5J3gwtz17a Nwng== X-Gm-Message-State: ACrzQf36D6jBurCSUiWFmlBLQmwlWsc+JKkQqPQFWIGQTKE0x/BjICz6 JMTzq2pIXIu2tDO3Cr/+r0UDB8LByJhvo0aR0ParUeFRzNbJpg== X-Google-Smtp-Source: AMsMyM5KMVQhSrYZKcp9wMwV0JEV6HVjjMOKl2ObRfylWKMxmt8+mI1as09O07TJUOjYmbcqY2DruYfYtHTFEpySNTg= X-Received: by 2002:ac8:7d04:0:b0:39c:fa3f:c917 with SMTP id g4-20020ac87d04000000b0039cfa3fc917mr422904qtb.320.1666110797015; Tue, 18 Oct 2022 09:33:17 -0700 (PDT) MIME-Version: 1.0 References: <98CBD80474FA8B44BF855DF32C47DC35D86DB2@smartserver.smartshare.dk> <20221009133737.795377-1-andrew.rybchenko@oktetlabs.ru> <20221009133737.795377-4-andrew.rybchenko@oktetlabs.ru> <98CBD80474FA8B44BF855DF32C47DC35D873B6@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D873B8@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D873E3@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D873E6@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D873E6@smartserver.smartshare.dk> From: Jerin Jacob Date: Tue, 18 Oct 2022 22:02:50 +0530 Message-ID: Subject: Re: [PATCH v6 3/4] mempool: fix cache flushing algorithm To: =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: Olivier Matz , Andrew Rybchenko , dev@dpdk.org, Bruce Richardson Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Sat, Oct 15, 2022 at 12:27 PM Morten Br=C3=B8rup wrote: > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > Sent: Friday, 14 October 2022 21.51 > > > > On Fri, Oct 14, 2022 at 05:57:39PM +0200, Morten Br=C3=B8rup wrote: > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com] > > > > Sent: Friday, 14 October 2022 16.01 > > > > > > > > Hi Morten, Andrew, > > > > > > > > On Sun, Oct 09, 2022 at 05:08:39PM +0200, Morten Br=C3=B8rup wrote: > > > > > > From: Andrew Rybchenko [mailto:andrew.rybchenko@oktetlabs.ru] > > > > > > Sent: Sunday, 9 October 2022 16.52 > > > > > > > > > > > > On 10/9/22 17:31, Morten Br=C3=B8rup wrote: > > > > > > >> From: Andrew Rybchenko > > [mailto:andrew.rybchenko@oktetlabs.ru] > > > > > > >> Sent: Sunday, 9 October 2022 15.38 > > > > > > >> > > > > > > >> From: Morten Br=C3=B8rup > > > > > > >> > > > > > > > > > > [...] > > > > > > > > I finally took a couple of hours to carefully review the mempool- > > > > related > > > > series (including the ones that have already been pushed). > > > > > > > > The new behavior looks better to me in all situations I can think > > > > about. > > > > > > Extreme care is required when touching a core library like the > > mempool. > > > > > > Thank you, Olivier. > > > > > > > > > > > > > > > > > > >> --- a/lib/mempool/rte_mempool.h > > > > > > >> +++ b/lib/mempool/rte_mempool.h > > > > > > >> @@ -90,7 +90,7 @@ struct rte_mempool_cache { > > > > > > >> * Cache is allocated to this size to allow it to > > overflow > > > > in > > > > > > >> certain > > > > > > >> * cases to avoid needless emptying of cache. > > > > > > >> */ > > > > > > >> - void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< > > Cache > > > > objects */ > > > > > > >> + void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 2]; /**< > > Cache > > > > objects */ > > > > > > >> } __rte_cache_aligned; > > > > > > > > > > > > > > How much are we allowed to break the ABI here? > > > > > > > > > > > > > > This patch reduces the size of the structure by removing a > > now > > > > unused > > > > > > part at the end, which should be harmless. > > > > > > > > It is an ABI breakage: an existing application will use the new > > 22.11 > > > > function to create the mempool (with a smaller cache), but will use > > the > > > > old inlined get/put that can exceed MAX_SIZE x 2 will remain. > > > > > > > > But this is a nice memory consumption improvement, in my opinion we > > > > should accept it for 22.11 with an entry in the release note. > > > > > > > > > > > > > > > > > > > > > > If we may also move the position of the objs array, I would > > add > > > > > > __rte_cache_aligned to the objs array. It makes no difference > > in > > > > the > > > > > > general case, but if get/put operations are always 32 objects, > > it > > > > will > > > > > > reduce the number of memory (or last level cache) accesses from > > > > five to > > > > > > four 64 B cache lines for every get/put operation. > > > > > > > > Will it really be the case? Since cache->len has to be accessed > > too, > > > > I don't think it would make a difference. > > > > > > Yes, the first cache line, containing cache->len, will be accessed > > always. I forgot to count that; so the improvement by aligning cache- > > >objs will be five cache line accesses instead of six. > > > > > > Let me try to explain the scenario in other words: > > > > > > In an application where a mempool cache is only accessed in bursts of > > 32 objects (256 B), it matters if those 256 B accesses in the mempool > > cache start at a cache line aligned address or not. If cache line > > aligned, accessing those 256 B in the mempool cache will only touch 4 > > cache lines; if not, 5 cache lines will be touched. (For architectures > > with 128 B cache line, it will be 2 instead of 3 touched cache lines > > per mempool cache get/put operation in applications using only bursts > > of 32 objects.) > > > > > > If we cache line align cache->objs, those bursts of 32 objects (256 > > B) will be cache line aligned: Any address at cache->objs[N * 32 > > objects] is cache line aligned if objs->objs[0] is cache line aligned. > > > > > > Currently, the cache->objs directly follows cache->len, which makes > > cache->objs[0] cache line unaligned. > > > > > > If we decide to break the mempool cache ABI, we might as well include > > my suggested cache line alignment performance improvement. It doesn't > > degrade performance for mempool caches not only accessed in bursts of > > 32 objects. > > > > I don't follow you. Currently, with 16 objects (128B), we access to 3 > > cache lines: > > > > =E2=94=8C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=90 > > =E2=94=82len =E2=94=82 > > cache =E2=94=82********=E2=94=82--- > > line0 =E2=94=82********=E2=94=82 ^ > > =E2=94=82********=E2=94=82 | > > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=A4 | 16 objects > > =E2=94=82********=E2=94=82 | 128B > > cache =E2=94=82********=E2=94=82 | > > line1 =E2=94=82********=E2=94=82 | > > =E2=94=82********=E2=94=82 | > > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=A4 | > > =E2=94=82********=E2=94=82_v_ > > cache =E2=94=82 =E2=94=82 > > line2 =E2=94=82 =E2=94=82 > > =E2=94=82 =E2=94=82 > > =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=98 > > > > With the alignment, it is also 3 cache lines: > > > > =E2=94=8C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=90 > > =E2=94=82len =E2=94=82 > > cache =E2=94=82 =E2=94=82 > > line0 =E2=94=82 =E2=94=82 > > =E2=94=82 =E2=94=82 > > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=A4--- > > =E2=94=82********=E2=94=82 ^ > > cache =E2=94=82********=E2=94=82 | > > line1 =E2=94=82********=E2=94=82 | > > =E2=94=82********=E2=94=82 | > > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=A4 | 16 objects > > =E2=94=82********=E2=94=82 | 128B > > cache =E2=94=82********=E2=94=82 | > > line2 =E2=94=82********=E2=94=82 | > > =E2=94=82********=E2=94=82 v > > =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80= =E2=94=80=E2=94=80=E2=94=98--- > > > > > > Am I missing something? > > Accessing the objects at the bottom of the mempool cache is a special cas= e, where cache line0 is also used for objects. > > Consider the next burst (and any following bursts): > > Current: > =E2=94=8C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=90 > =E2=94=82len =E2=94=82 > cache =E2=94=82 =E2=94=82 > line0 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 > =E2=94=82 =E2=94=82 > cache =E2=94=82 =E2=94=82 > line1 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 > =E2=94=82 =E2=94=82 > cache =E2=94=82********=E2=94=82--- > line2 =E2=94=82********=E2=94=82 ^ > =E2=94=82********=E2=94=82 | > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 | 16 objects > =E2=94=82********=E2=94=82 | 128B > cache =E2=94=82********=E2=94=82 | > line3 =E2=94=82********=E2=94=82 | > =E2=94=82********=E2=94=82 | > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 | > =E2=94=82********=E2=94=82_v_ > cache =E2=94=82 =E2=94=82 > line4 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=98 > 4 cache lines touched, incl. line0 for len. > > With the proposed alignment: > =E2=94=8C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=90 > =E2=94=82len =E2=94=82 > cache =E2=94=82 =E2=94=82 > line0 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 > =E2=94=82 =E2=94=82 > cache =E2=94=82 =E2=94=82 > line1 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 > =E2=94=82 =E2=94=82 > cache =E2=94=82 =E2=94=82 > line2 =E2=94=82 =E2=94=82 > =E2=94=82 =E2=94=82 > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 > =E2=94=82********=E2=94=82--- > cache =E2=94=82********=E2=94=82 ^ > line3 =E2=94=82********=E2=94=82 | > =E2=94=82********=E2=94=82 | 16 objects > =E2=94=9C=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=A4 | 128B > =E2=94=82********=E2=94=82 | > cache =E2=94=82********=E2=94=82 | > line4 =E2=94=82********=E2=94=82 | > =E2=94=82********=E2=94=82_v_ > =E2=94=94=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2= =94=80=E2=94=80=E2=94=98 > Only 3 cache lines touched, incl. line0 for len. When tested with testpmd,l3fwd there was less than 1% regression. It could be noise. But making the cacheline alignment is fixing that. In addition to @Morten Br=C3=B8rup point, I think, there is a factor "load" stall on cache->len read, What I meant by that is: In the case of (len and objs) are in the same cache line. Assume objs are written as stores operation and not read anything on cacheline VS a few stores done for objects and on subsequent len read via enqueue operation may stall based where those obj reached in the cache hierarchy and cache policy(write-back vs write-through) If we are seeing no regression with cachealinged with various platform testing then I think it make sense to make cache aligned. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > uint32_t len; /**< Current cache count */ > > > > > > > - /* > > > > > > > - * Cache is allocated to this size to allow it to overflo= w > > > > in > > > > > > certain > > > > > > > - * cases to avoid needless emptying of cache. > > > > > > > - */ > > > > > > > - void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache > > > > objects */ > > > > > > > + /** > > > > > > > + * Cache objects > > > > > > > + * > > > > > > > + * Cache is allocated to this size to allow it to overflo= w > > > > in > > > > > > certain > > > > > > > + * cases to avoid needless emptying of cache. > > > > > > > + */ > > > > > > > + void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 2] > > > > __rte_cache_aligned; > > > > > > > } __rte_cache_aligned; > > > > > > > > > > > > I think aligning objs on cacheline should be a separate patch. > > > > > > > > > > Good point. I'll let you do it. :-) > > > > > > > > > > PS: Thank you for following up on this patch series, Andrew! > > > > > > > > Many thanks for this rework. > > > > > > > > Acked-by: Olivier Matz > > > > > > Perhaps Reviewed-by would be appropriate? > > > > I was thinking that "Acked-by" was commonly used by maintainers, and > > "Reviewed-by" for reviews by community members. After reading the > > documentation again, it's not that clear now in my mind :) > > > > Thanks, > > Olivier >