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 DB6B5A0093; Tue, 8 Nov 2022 18:38:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CC6DB40156; Tue, 8 Nov 2022 18:38:07 +0100 (CET) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id D1C33400D7 for ; Tue, 8 Nov 2022 18:38:06 +0100 (CET) Received: from frapeml100007.china.huawei.com (unknown [172.18.147.226]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4N6Fcl51Rbz67MmR; Wed, 9 Nov 2022 01:35:51 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml100007.china.huawei.com (7.182.85.133) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 8 Nov 2022 18:38:05 +0100 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2375.031; Tue, 8 Nov 2022 18:38:05 +0100 From: Konstantin Ananyev To: Bruce Richardson , Thomas Monjalon CC: =?iso-8859-1?Q?Morten_Br=F8rup?= , "andrew.rybchenko@oktetlabs.ru" , "olivier.matz@6wind.com" , "david.marchand@redhat.com" , "dev@dpdk.org" , "hofors@lysator.liu.se" , "mattias.ronnblom@ericsson.com" , "stephen@networkplumber.org" , "jerinj@marvell.com" Subject: RE: FW: [PATCH v4 3/3] mempool: use cache for frequently updated stats Thread-Topic: FW: [PATCH v4 3/3] mempool: use cache for frequently updated stats Thread-Index: AQHY8EWGJHVmHQn4qECYwEvekL7ida40xLXwgAAcq8CAAAcqEIAAEz2AgAAQHICAABangIAAAkcAgAArPdA= Date: Tue, 8 Nov 2022 17:38:04 +0000 Message-ID: <74775cadd8174c3797b4076929ebbcb7@huawei.com> References: <98CBD80474FA8B44BF855DF32C47DC35D874A1@smartserver.smartshare.dk> <6110999.17fYzF0512@thomas> <98CBD80474FA8B44BF855DF32C47DC35D874A3@smartserver.smartshare.dk> <2028060.trqCLbgVIZ@thomas> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.195.32.54] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-CFilter-Loop: Reflected 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 >=20 > On Tue, Nov 08, 2022 at 04:51:11PM +0100, Thomas Monjalon wrote: > > 08/11/2022 15:30, Morten Br=F8rup: > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > 08/11/2022 12:25, Morten Br=F8rup: > > > > > From: Morten Br=F8rup > > > > > > From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com] > > > > > > Sent: Tuesday, 8 November 2022 10.20 > > > > > > > +#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 > > > > > > > > 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 conditi= on). > > > > > > Short version: With this series, mempool statistics can be used in pr= oduction. Without it, the performance cost > (mempool_perf_autotest: -74 %) is prohibitive! > > > > > > Long version: > > > > > > The patch series provides significantly higher performance for mempoo= l 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 buil= d 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 perform= ance 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 sett= ing 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_DEB= UG or RTE_LIBRTE_MEMPOOL_STATS are set when > building. They are not set in the default build. > > > > If theses build flags are not set, there is no risk and no benefit. > > But if they are set, there is a risk of regression, > > for the benefit of an increased performance of a debug feature. > > I would say it is better to avoid any functional regression in a debug = feature > > at this stage. > > Any other opinion? > > > While I agree that we should avoid any functional regression, I wonder ho= w > widely used the debug feature is, and how big the risk of a regression is= ? > Even if there is one, having a regression in a debug feature is a lot les= s > serious than having one in something which goes into production. >=20 Unless it introduces an ABI breakage (as I understand it doesn't), I'll wai= t till 23.03. Just in case. BTW, as a side thought - if the impact is really that small now, would it m= ake sense to make it run-time option, instead of compile-time one? Konstantin