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 71E45A0542; Mon, 29 Aug 2022 12:53:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1AF0D4069D; Mon, 29 Aug 2022 12:53:30 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by mails.dpdk.org (Postfix) with ESMTP id DF9C54003C for ; Mon, 29 Aug 2022 12:53:28 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 9322C5C00FF; Mon, 29 Aug 2022 06:53:28 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Mon, 29 Aug 2022 06:53:28 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1661770408; x= 1661856808; bh=6Z2MSqc7QmbieGDa1Oty2gF4MD4hRtBijc9xJK4OCFQ=; b=g FFvDWPBqFtIk+vVWlMluwFYGNy18J5p1OsYjnjXGDmX6jsn9/lavoyZKblSMKtyS ni9mz5AKf+phE+KAC0zBdFOeuw/L3EAZjANjuyeJ7jbb52Tz9lbw/1nhnIFg1gXN gU42jORmZpnCKq9q8Wxu0Zb407+XGIOuupI/7Hbmkw2zIQRXwfvtqG6ErXKwJs5E 72Tt41fB+zpxbgaiekNyton8fTdyJp9Q95XdIqYkwX7+F3WWaGR9qmhOnYz6eGSe 146u1UjLwyZtidDLmlWMcwucpnFU1uj3Lo+lsE210m5BtrgMkk105SjOFG2qtK3v GoRp3/metCAK8zJukd+FA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1661770408; x= 1661856808; bh=6Z2MSqc7QmbieGDa1Oty2gF4MD4hRtBijc9xJK4OCFQ=; b=e +CDPWYoQ15P9dFWNN5BBO4l4qnWfmwzAsPD9hKIwaRLNoQz3OZrhTiEiWPWqWFyr JP4uNA21bYUDSrmG6sfp3yNK+DdmpkHBprkFBUBA/GnGASAWhaFwIudNNLO75MwH 7gbPGdI3wgDvsYUUwPIm8bUEKETCxYZC8WS9DBhFeooA+a4vhxSBE05hTDrfSE3Q cW9xgM2Cx8jMXMF3ATayDoDDUiWAiqNq1qHW5+hBNeN0AyBLdhNvacuW+OqdLbJ6 wMKxTK9MZ3W8QOMHCpW7vTvCQLg/cqPcoXEsI06qoftmwEI+I2E5ieP87O1kFq90 ClQviZLUx/gsX+AGUo56Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrvdekuddgfeehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtudenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeegjeeivdelheduhfeugfefvefgheeiteegiefggffgffffieeu gfeukedtfeefveenucffohhmrghinhepughpughkrdhorhhgnecuvehluhhsthgvrhfuih iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhho nhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 29 Aug 2022 06:53:25 -0400 (EDT) From: Thomas Monjalon To: Morten =?ISO-8859-1?Q?Br=F8rup?= , Bruce Richardson Cc: Kevin Laatz , Jerin Jacob , Anatoly Burakov , dpdk-dev , Conor Walsh , David Hunt , Nicolas Chautru , Fan Zhang , Ashish Gupta , Akhil Goyal , Chengwen Feng , Ray Kinsella , Ferruh Yigit , Andrew Rybchenko , Jerin Jacob , Sachin Saxena , Hemant Agrawal , Ori Kam , Honnappa Nagarahalli , Konstantin Ananyev Subject: Re: [PATCH v3 1/3] eal: add lcore poll busyness telemetry Date: Mon, 29 Aug 2022 12:53:24 +0200 Message-ID: <2555175.9Mp67QZiUf@thomas> In-Reply-To: References: <24c49429394294cfbf0d9c506b205029bac77c8b.1657890378.git.anatoly.burakov@intel.com> <98CBD80474FA8B44BF855DF32C47DC35D872C4@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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 29/08/2022 12:41, Bruce Richardson: > On Fri, Aug 26, 2022 at 05:46:48PM +0200, Morten Br=F8rup wrote: > > > From: Kevin Laatz [mailto:kevin.laatz@intel.com] > > > Sent: Friday, 26 August 2022 17.27 > > >=20 > > > On 26/08/2022 09:29, Morten Br=F8rup wrote: > > > >> From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > > > >> Sent: Friday, 26 August 2022 10.16 > > > >> > > > >> On Fri, Aug 26, 2022 at 1:37 PM Bruce Richardson > > > >> wrote: > > > >>> On Fri, Aug 26, 2022 at 12:35:16PM +0530, Jerin Jacob wrote: > > > >>>> On Thu, Aug 25, 2022 at 8:56 PM Kevin Laatz > > > > > > >> wrote: > > > >>>>> From: Anatoly Burakov > > > >>>>> > > > >>>>> Currently, there is no way to measure lcore poll busyness in a > > > >> passive way, > > > >>>>> without any modifications to the application. This patch adds a > > > >> new EAL API > > > >>>>> that will be able to passively track core polling busyness. > > > >>>>> > > > >>>>> The poll busyness is calculated by relying on the fact that most > > > >> DPDK API's > > > >>>>> will poll for packets. Empty polls can be counted as "idle", > > > >> while > > > >>>>> non-empty polls can be counted as busy. To measure lcore poll > > > >> busyness, we > > > >>>>> simply call the telemetry timestamping function with the number > > > >> of polls a > > > >>>>> particular code section has processed, and count the number of > > > >> cycles we've > > > >>>>> spent processing empty bursts. The more empty bursts we > > > >> encounter, the less > > > >>>>> cycles we spend in "busy" state, and the less core poll busyness > > > >> will be > > > >>>>> reported. > > > >>>>> > > > >>>>> In order for all of the above to work without modifications to > > > >> the > > > >>>>> application, the library code needs to be instrumented with cal= ls > > > >> to the > > > >>>>> lcore telemetry busyness timestamping function. The following > > > >> parts of DPDK > > > >>>>> are instrumented with lcore telemetry calls: > > > >>>>> > > > >>>>> - All major driver API's: > > > >>>>> - ethdev > > > >>>>> - cryptodev > > > >>>>> - compressdev > > > >>>>> - regexdev > > > >>>>> - bbdev > > > >>>>> - rawdev > > > >>>>> - eventdev > > > >>>>> - dmadev > > > >>>>> - Some additional libraries: > > > >>>>> - ring > > > >>>>> - distributor > > > >>>>> > > > >>>>> To avoid performance impact from having lcore telemetry support, > > > >> a global > > > >>>>> variable is exported by EAL, and a call to timestamping function > > > >> is wrapped > > > >>>>> into a macro, so that whenever telemetry is disabled, it only > > > >> takes one > > > >>>>> additional branch and no function calls are performed. It is al= so > > > >> possible > > > >>>>> to disable it at compile time by commenting out > > > >> RTE_LCORE_BUSYNESS from > > > >>>>> build config. > > > >>>>> > > > >>>>> This patch also adds a telemetry endpoint to report lcore poll > > > >> busyness, as > > > >>>>> well as telemetry endpoints to enable/disable lcore telemetry. A > > > >>>>> documentation entry has been added to the howto guides to expla= in > > > >> the usage > > > >>>>> of the new telemetry endpoints and API. > > > >>>>> > > > >>>>> Signed-off-by: Kevin Laatz > > > >>>>> Signed-off-by: Conor Walsh > > > >>>>> Signed-off-by: David Hunt > > > >>>>> Signed-off-by: Anatoly Burakov > > > >>>>> > > > >>>>> --- > > > >>>>> v3: > > > >>>>> * Fix missed renaming to poll busyness > > > >>>>> * Fix clang compilation > > > >>>>> * Fix arm compilation > > > >>>>> > > > >>>>> v2: > > > >>>>> * Use rte_get_tsc_hz() to adjust the telemetry period > > > >>>>> * Rename to reflect polling busyness vs general busyness > > > >>>>> * Fix segfault when calling telemetry timestamp from an > > > >> unregistered > > > >>>>> non-EAL thread. > > > >>>>> * Minor cleanup > > > >>>>> --- > > > >>>>> diff --git a/meson_options.txt b/meson_options.txt > > > >>>>> index 7c220ad68d..725b851f69 100644 > > > >>>>> --- a/meson_options.txt > > > >>>>> +++ b/meson_options.txt > > > >>>>> @@ -20,6 +20,8 @@ option('enable_driver_sdk', type: 'boolean', > > > >> value: false, description: > > > >>>>> 'Install headers to build drivers.') > > > >>>>> option('enable_kmods', type: 'boolean', value: false, > > > >> description: > > > >>>>> 'build kernel modules') > > > >>>>> +option('enable_lcore_poll_busyness', type: 'boolean', value: > > > >> true, description: > > > >>>>> + 'enable collection of lcore poll busyness telemetry') > > > >>>> IMO, All fastpath features should be opt-in. i.e default should = be > > > >> false. > > > >>>> For the trace fastpath related changes, We have done the similar > > > >> thing > > > >>>> even though it cost additional one cycle for disabled trace poin= ts > > > >>>> > > > >>> We do need to consider runtime and build defaults differently, > > > >> though. > > > >>> Since this has also runtime enabling, I think having build-time > > > >> enabling > > > >>> true as default is ok, so long as the runtime enabling is false > > > >> (assuming > > > >>> no noticable overhead when the feature is disabled.) > > > >> I was talking about buildtime only. "enable_trace_fp" meson option > > > >> selected as > > > >> false as default. > > > > Agree. "enable_lcore_poll_busyness" is in the fast path, so it shou= ld > > > follow the design pattern of "enable_trace_fp". > > >=20 > > > +1 to making this opt-in. However, I'd lean more towards having the > > > buildtime option enabled and the runtime option disabled by default. > > > There is no measurable impact cause by the extra branch (the check for > > > enabled/disabled in the macro) by disabling at runtime, and we gain t= he > > > benefit of avoiding a recompile to enable it later. > >=20 > > The exact same thing could be said about "enable_trace_fp"; however, th= e development effort was put into separating it from "enable_trace", so it = could be disabled by default. > >=20 > > Your patch is unlikely to get approved if you don't follow the "enable_= trace_fp" design pattern as suggested. > >=20 > > >=20 > > > > > > > >> If the concern is enabling on generic distros then distro generic > > > >> config can opt in this > > > >> > > > >>> /Bruce > > > > @Kevin, are you considering a roadmap for using > > > RTE_LCORE_TELEMETRY_TIMESTAMP() for other purposes? Otherwise, it > > > should also be renamed to indicate that it is part of the "poll > > > busyness" telemetry. > > >=20 > > > No further purposes are planned for this macro, I'll rename it in the > > > next revision. > >=20 > > OK. Thank you. > >=20 > > Also, there's a new discussion about EAL bloat [1]. Perhaps I'm stretch= ing it here, but it would be nice if your library was made a separate libra= ry, instead of part of the EAL library. (Since this kind of feature is not = new to the EAL, I will categorize this suggestion as "nice to have", not "m= ust have".) > >=20 > > [1] http://inbox.dpdk.org/dev/2594603.Isy0gbHreE@thomas/T/ > >=20 >=20 > I was actually discussing this with Kevin and Dave H. on Friay, and trying > to make this a separate library is indeed a big stretch. :-) >=20 > From that discussion, the key point/gap is that we are really missing a > clean way of providing undefs or macro fallbacks for when a library is ju= st > not present. For example, if this was a separate library we would gain a > number of advantages e.g. no need for separate enable/disable flag, but t= he > big disadvantage is that every header include for it, and every reference > to the macros used in that header need to be surrounded by big ugly ifdef= s. >=20 > For now, adding this into EAL is the far more practical approach, since it > means that even if support for the feature is disabled at build time the > header is still available to provide the appropriate no-op macros. We can make the library always available with different implementations based on a build option. But is it a good idea to have a different behaviour based on build option? Why not making it a runtime option? Can the performance hit be cached in some way?