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 48CB4A0093; Tue, 8 Nov 2022 15:30:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D54AC400D4; Tue, 8 Nov 2022 15:30:10 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 528004003C for ; Tue, 8 Nov 2022 15:30:09 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: FW: [PATCH v4 3/3] mempool: use cache for frequently updated stats Date: Tue, 8 Nov 2022 15:30:07 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D874A3@smartserver.smartshare.dk> In-Reply-To: <6110999.17fYzF0512@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: FW: [PATCH v4 3/3] mempool: use cache for frequently updated stats Thread-Index: AdjzdpA9b6ZFB74eQ4CsPtbxhIsGFwAAJlQA References: <98CBD80474FA8B44BF855DF32C47DC35D874A1@smartserver.smartshare.dk> <6110999.17fYzF0512@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , , Cc: , , , , "Konstantin Ananyev" , , , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Tuesday, 8 November 2022 14.32 >=20 > 08/11/2022 12:25, Morten Br=F8rup: > > From: Morten Br=F8rup > > Sent: Tuesday, 8 November 2022 12.22 > > > > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com] > > > Sent: Tuesday, 8 November 2022 10.20 > > > > > > > When built with stats enabled (RTE_LIBRTE_MEMPOOL_STATS = defined), > the > > > > performance of mempools with caches is improved as follows. > > > > > > > > When accessing objects in the mempool, either the put_bulk and > > > put_objs or > > > > the get_success_bulk and get_success_objs statistics counters = are > > > likely > > > > to be incremented. > > > > > > > > By adding an alternative set of these counters to the mempool > cache > > > > structure, accessing the dedicated statistics structure is > avoided in > > > the > > > > likely cases where these counters are incremented. > > > > > > > > The trick here is that the cache line holding the mempool cache > > > structure > > > > is accessed anyway, in order to access the 'len' or = 'flushthresh' > > > fields. > > > > Updating some statistics counters in the same cache line has > lower > > > > performance cost than accessing the statistics counters in the > > > dedicated > > > > statistics structure, which resides in another cache line. > > > > > > > > mempool_perf_autotest with this patch shows the following > > > improvements in > > > > rate_persec. > > > > > > > > The cost of enabling mempool stats (without debug) after this > patch: > > > > -6.8 % and -6.7 %, respectively without and with cache. > > > > > > > > v4: > > > > * Fix checkpatch warnings: > > > > A couple of typos in the patch description. > > > > The macro to add to a mempool cache stat variable should not > use > > > > do {} while (0). Personally, I would tend to disagree with > this, > > > but > > > > whatever keeps the CI happy. > > > > v3: > > > > * Don't update the description of the RTE_MEMPOOL_STAT_ADD = macro. > > > > This change belongs in the first patch of the series. > > > > v2: > > > > * Move the statistics counters into a stats structure. > > > > > > > > Signed-off-by: Morten Br=F8rup > > > > --- > > > > [...] > > > > > > +/** > > > > + * @internal When stats is enabled, store some statistics. > > > > + * > > > > + * @param cache > > > > + * Pointer to the memory pool cache. > > > > + * @param name > > > > + * Name of the statistics field to increment in the memory > pool > > > cache. > > > > + * @param n > > > > + * Number to add to the statistics. > > > > + */ > > > > +#ifdef RTE_LIBRTE_MEMPOOL_STATS > > > > +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) (cache)- > >stats.name +=3D n > > > > > > As Andrew already pointed, it needs to be: ((cache)->stats.name = +=3D > (n)) > > > Apart from that, LGTM. > > > Series-Acked-by: Konstantin Ananyev = > > > > @Thomas, this series should be ready to apply... it now has been: > > Reviewed-by: (mempool maintainer) Andrew Rybchenko > > > Reviewed-By: Mattias R=F6nnblom > > Acked-by: Konstantin Ananyev >=20 > Being acked does not mean it is good to apply in -rc3. I understand that the RFC/v1 of this series was formally too late to = make it in 22.11, so I will not complain loudly if you choose to omit it = for 22.11. With two independent reviews, including from a mempool maintainer, I = still have some hope. Also considering the risk assessment below. ;-) > Please tell what is the benefit for 22.11 (before/after and = condition). Short version: With this series, mempool statistics can be used in = production. Without it, the performance cost (mempool_perf_autotest: -74 = %) is prohibitive! Long version: The patch series provides significantly higher performance for mempool = statistics, which are readable through rte_mempool_dump(FILE *f, struct = rte_mempool *mp). Without this series, you have to set RTE_LIBRTE_MEMPOOL_DEBUG at build = time to get mempool statistics. RTE_LIBRTE_MEMPOOL_DEBUG also enables = protective cookies before and after each mempool object, which are all = verified on get/put from the mempool. According to = mempool_perf_autotest, the performance cost of mempool statistics (by = setting RTE_LIBRTE_MEMPOOL_DEBUG) is a 74 % decrease in rate_persec for = mempools with cache (i.e. mbuf pools). Prohibitive for use in = production! With this series, the performance cost of mempool statistics (by setting = RTE_LIBRTE_MEMPOOL_STATS) in mempool_perf_autotest is only 6.7 %, so = mempool statistics can be used in production. > Note there is a real risk doing such change that late. Risk assessment: The patch series has zero effect unless either RTE_LIBRTE_MEMPOOL_DEBUG = or RTE_LIBRTE_MEMPOOL_STATS are set when building. They are not set in = the default build. >=20 > > Please fix the RTE_MEMPOOL_CACHE_STAT_ADD macro while merging, to > satisfy checkpatch. ;-) > > > > It should be: > > > > +#ifdef RTE_LIBRTE_MEMPOOL_STATS > > +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) ((cache)- > >stats.name +=3D (n)) > > +#else > > +#define RTE_MEMPOOL_CACHE_STAT_ADD(cache, name, n) do {} while (0) > > +#endif >=20 > Would be easier if you fix it. I will send a v5 of the series. >=20 > > @Thomas/@David: I changed the state of this patch series to Awaiting > Upstream in patchwork. Is that helpful, or should I change them to = some > other state? >=20 > You should keep it as "New". OK. Thank you.