| Message ID | 20230110234642.1188550-1-tduszynski@marvell.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2620542395; Wed, 11 Jan 2023 00:46:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E8D5D40E25; Wed, 11 Jan 2023 00:46:55 +0100 (CET) Received: from mx0b-0016f401.pphosted.com (mx0b-0016f401.pphosted.com [67.231.156.173]) by mails.dpdk.org (Postfix) with ESMTP id E6B084021E for <dev@dpdk.org>; Wed, 11 Jan 2023 00:46:53 +0100 (CET) Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 30ANJJD9003867; Tue, 10 Jan 2023 15:46:50 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-transfer-encoding : content-type; s=pfpt0220; bh=XqeYJKByabssNYzTAXGiCbCKEfcne+epFChIa201tNU=; b=aclQ1zsKqkFM+tJZ+q/V6EDUjIIO2pPruwEL750pdgBc29PjvqZIeSmp2dgLAlph+ivG WgDQuEjdvRgUOKiFwetc0LaUMVTTlLFQvUoz2OOhED0UpKhT3/Affj5PauzOVRh3JDgA EjYyHrG1Td+cEO7d7PTvKN460MJLzTudLuKWjlm7FiVv2B/iI4hYuhda1pxFjH+cGmVQ /t139Qk6E02chmFOIea01+LSxEFVS88nTb7E1HhnpOqR0zKUo61p+0cuO8TzqXEgxnAl oz3gLMe9IVVMe3AfuppN/eA3bOBLM+DyJoTjlzIiND32kFwmxVc6lJ7Bs2593LRoTEOn ig== Received: from dc5-exch02.marvell.com ([199.233.59.182]) by mx0b-0016f401.pphosted.com (PPS) with ESMTPS id 3my94twnvx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Tue, 10 Jan 2023 15:46:50 -0800 Received: from DC5-EXCH02.marvell.com (10.69.176.39) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Tue, 10 Jan 2023 15:46:47 -0800 Received: from maili.marvell.com (10.69.176.80) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server id 15.0.1497.42 via Frontend Transport; Tue, 10 Jan 2023 15:46:47 -0800 Received: from cavium-DT10.. (unknown [10.28.34.39]) by maili.marvell.com (Postfix) with ESMTP id ACC1E3F7066; Tue, 10 Jan 2023 15:46:44 -0800 (PST) From: Tomasz Duszynski <tduszynski@marvell.com> To: <dev@dpdk.org> CC: <thomas@monjalon.net>, <jerinj@marvell.com>, <mb@smartsharesystems.com>, <Ruifeng.Wang@arm.com>, <mattias.ronnblom@ericsson.com>, <zhoumin@loongson.cn>, Tomasz Duszynski <tduszynski@marvell.com> Subject: [PATCH v5 0/4] add support for self monitoring Date: Wed, 11 Jan 2023 00:46:38 +0100 Message-ID: <20230110234642.1188550-1-tduszynski@marvell.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20221213104350.3218167-1-tduszynski@marvell.com> References: <20221213104350.3218167-1-tduszynski@marvell.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Proofpoint-ORIG-GUID: wBcsSeXp7cglnT83KXMw8q2jLIs0taYs X-Proofpoint-GUID: wBcsSeXp7cglnT83KXMw8q2jLIs0taYs X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2023-01-10_09,2023-01-10_03,2022-06-22_01 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
| Series | add support for self monitoring | |
Message
Tomasz Duszynski
Jan. 10, 2023, 11:46 p.m. UTC
This series adds self monitoring support i.e allows to configure and read performance measurement unit (PMU) counters in runtime without using perf utility. This has certain adventages when application runs on isolated cores with nohz_full kernel parameter. Events can be read directly using rte_pmu_read() or using dedicated tracepoint rte_eal_trace_pmu_read(). The latter will cause events to be stored inside CTF file. By design, all enabled events are grouped together and the same group is attached to lcores that use self monitoring funtionality. Events are enabled by names, which need to be read from standard location under sysfs i.e /sys/bus/event_source/devices/PMU/events where PMU is a core pmu i.e one measuring cpu events. As of today raw events are not supported. v5: - address review comments - fix sign extension while reading pmu on x86 - fix regex mentioned in doc - various minor changes/improvements here and there v4: - fix freeing mem detected by debug_autotest v3: - fix shared build v2: - fix problems reported by test build infra Tomasz Duszynski (4): eal: add generic support for reading PMU events eal/arm: support reading ARM PMU events in runtime eal/x86: support reading Intel PMU events in runtime eal: add PMU support to tracing library app/test/meson.build | 1 + app/test/test_pmu.c | 47 +++ app/test/test_trace_perf.c | 4 + doc/guides/prog_guide/profile_app.rst | 13 + doc/guides/prog_guide/trace_lib.rst | 32 ++ lib/eal/arm/include/meson.build | 1 + lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ lib/eal/arm/meson.build | 4 + lib/eal/arm/rte_pmu.c | 104 +++++ lib/eal/common/eal_common_trace_points.c | 3 + lib/eal/common/meson.build | 3 + lib/eal/common/pmu_private.h | 41 ++ lib/eal/common/rte_pmu.c | 504 +++++++++++++++++++++++ lib/eal/include/meson.build | 1 + lib/eal/include/rte_eal_trace.h | 10 + lib/eal/include/rte_pmu.h | 202 +++++++++ lib/eal/linux/eal.c | 4 + lib/eal/version.map | 7 + lib/eal/x86/include/meson.build | 1 + lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ 20 files changed, 1054 insertions(+) create mode 100644 app/test/test_pmu.c create mode 100644 lib/eal/arm/include/rte_pmu_pmc.h create mode 100644 lib/eal/arm/rte_pmu.c create mode 100644 lib/eal/common/pmu_private.h create mode 100644 lib/eal/common/rte_pmu.c create mode 100644 lib/eal/include/rte_pmu.h create mode 100644 lib/eal/x86/include/rte_pmu_pmc.h -- 2.34.1
Comments
hi, don't interpret this as an objection to the functionality but this looks like a clear example of something that doesn't belong in the EAL. has there been a discussion as to whether or not this should be in a separate library? a basic test is whether or not an implementation exists or can be reasonably provided for all platforms and that isn't strictly evident here. red flag is to see yet more code being added conditionally compiled for a single platform. Morten, Bruce comments? thanks On Wed, Jan 11, 2023 at 12:46:38AM +0100, Tomasz Duszynski wrote: > This series adds self monitoring support i.e allows to configure and > read performance measurement unit (PMU) counters in runtime without > using perf utility. This has certain adventages when application runs on > isolated cores with nohz_full kernel parameter. > > Events can be read directly using rte_pmu_read() or using dedicated > tracepoint rte_eal_trace_pmu_read(). The latter will cause events to be > stored inside CTF file. > > By design, all enabled events are grouped together and the same group > is attached to lcores that use self monitoring funtionality. > > Events are enabled by names, which need to be read from standard > location under sysfs i.e > > /sys/bus/event_source/devices/PMU/events > > where PMU is a core pmu i.e one measuring cpu events. As of today > raw events are not supported. > > v5: > - address review comments > - fix sign extension while reading pmu on x86 > - fix regex mentioned in doc > - various minor changes/improvements here and there > v4: > - fix freeing mem detected by debug_autotest > v3: > - fix shared build > v2: > - fix problems reported by test build infra > > Tomasz Duszynski (4): > eal: add generic support for reading PMU events > eal/arm: support reading ARM PMU events in runtime > eal/x86: support reading Intel PMU events in runtime > eal: add PMU support to tracing library > > app/test/meson.build | 1 + > app/test/test_pmu.c | 47 +++ > app/test/test_trace_perf.c | 4 + > doc/guides/prog_guide/profile_app.rst | 13 + > doc/guides/prog_guide/trace_lib.rst | 32 ++ > lib/eal/arm/include/meson.build | 1 + > lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ > lib/eal/arm/meson.build | 4 + > lib/eal/arm/rte_pmu.c | 104 +++++ > lib/eal/common/eal_common_trace_points.c | 3 + > lib/eal/common/meson.build | 3 + > lib/eal/common/pmu_private.h | 41 ++ > lib/eal/common/rte_pmu.c | 504 +++++++++++++++++++++++ > lib/eal/include/meson.build | 1 + > lib/eal/include/rte_eal_trace.h | 10 + > lib/eal/include/rte_pmu.h | 202 +++++++++ > lib/eal/linux/eal.c | 4 + > lib/eal/version.map | 7 + > lib/eal/x86/include/meson.build | 1 + > lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ > 20 files changed, 1054 insertions(+) > create mode 100644 app/test/test_pmu.c > create mode 100644 lib/eal/arm/include/rte_pmu_pmc.h > create mode 100644 lib/eal/arm/rte_pmu.c > create mode 100644 lib/eal/common/pmu_private.h > create mode 100644 lib/eal/common/rte_pmu.c > create mode 100644 lib/eal/include/rte_pmu.h > create mode 100644 lib/eal/x86/include/rte_pmu_pmc.h > > -- > 2.34.1
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Wednesday, 11 January 2023 01.32 > > On Wed, Jan 11, 2023 at 12:46:38AM +0100, Tomasz Duszynski wrote: > > This series adds self monitoring support i.e allows to configure and > > read performance measurement unit (PMU) counters in runtime without > > using perf utility. This has certain adventages when application runs > on > > isolated cores with nohz_full kernel parameter. > > > > Events can be read directly using rte_pmu_read() or using dedicated > > tracepoint rte_eal_trace_pmu_read(). The latter will cause events to > be > > stored inside CTF file. > > > > By design, all enabled events are grouped together and the same group > > is attached to lcores that use self monitoring funtionality. > > > > Events are enabled by names, which need to be read from standard > > location under sysfs i.e > > > > /sys/bus/event_source/devices/PMU/events > > > > where PMU is a core pmu i.e one measuring cpu events. As of today > > raw events are not supported. > > > > v5: > > - address review comments > > - fix sign extension while reading pmu on x86 > > - fix regex mentioned in doc > > - various minor changes/improvements here and there > > v4: > > - fix freeing mem detected by debug_autotest > > v3: > > - fix shared build > > v2: > > - fix problems reported by test build infra > > > > Tomasz Duszynski (4): > > eal: add generic support for reading PMU events > > eal/arm: support reading ARM PMU events in runtime > > eal/x86: support reading Intel PMU events in runtime > > eal: add PMU support to tracing library > > > > app/test/meson.build | 1 + > > app/test/test_pmu.c | 47 +++ > > app/test/test_trace_perf.c | 4 + > > doc/guides/prog_guide/profile_app.rst | 13 + > > doc/guides/prog_guide/trace_lib.rst | 32 ++ > > lib/eal/arm/include/meson.build | 1 + > > lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ > > lib/eal/arm/meson.build | 4 + > > lib/eal/arm/rte_pmu.c | 104 +++++ > > lib/eal/common/eal_common_trace_points.c | 3 + > > lib/eal/common/meson.build | 3 + > > lib/eal/common/pmu_private.h | 41 ++ > > lib/eal/common/rte_pmu.c | 504 > +++++++++++++++++++++++ > > lib/eal/include/meson.build | 1 + > > lib/eal/include/rte_eal_trace.h | 10 + > > lib/eal/include/rte_pmu.h | 202 +++++++++ > > lib/eal/linux/eal.c | 4 + > > lib/eal/version.map | 7 + > > lib/eal/x86/include/meson.build | 1 + > > lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ > > 20 files changed, 1054 insertions(+) > > create mode 100644 app/test/test_pmu.c > > create mode 100644 lib/eal/arm/include/rte_pmu_pmc.h > > create mode 100644 lib/eal/arm/rte_pmu.c > > create mode 100644 lib/eal/common/pmu_private.h > > create mode 100644 lib/eal/common/rte_pmu.c > > create mode 100644 lib/eal/include/rte_pmu.h > > create mode 100644 lib/eal/x86/include/rte_pmu_pmc.h > > > > -- > > 2.34.1 [Moved Tyler's post down here.] > > hi, > > don't interpret this as an objection to the functionality but this > looks > like a clear example of something that doesn't belong in the EAL. has > there been a discussion as to whether or not this should be in a > separate library? IIRC, there has been no such discussion. Although I agree that this doesn't belong in EAL, I would point to the trace library as a reference for allowing it into the EAL. For the records, I also oppose to the trace library being part of the EAL. On the other hand, it would be interesting to determine if it is *impossible* adding this functionality as any other normal DPDK library, i.e. outside of the EAL, or if there is an unavoidable tie-in to the EAL. @Tomasz, if this is impossible, please describe the unavoidable tie-in to the EAL. No need for a long report, just a few words. You (and this functionality) shouldn't suffer from our long term ambition to move stuff out of the EAL. > > a basic test is whether or not an implementation exists or can be > reasonably provided for all platforms and that isn't strictly evident > here. red flag is to see yet more code being added conditionally > compiled for a single platform. Another basic test: Can DPDK applications run without it? If they can, an Environment Abstraction Layer does not need to have it, and thus it does not need to be part of the EAL. > > Morten, Bruce comments? > > thanks
Hi Tyler, >-----Original Message----- >From: Tyler Retzlaff <roretzla@linux.microsoft.com> >Sent: Wednesday, January 11, 2023 1:32 AM >To: Tomasz Duszynski <tduszynski@marvell.com>; bruce.richardson@intel.com; mb@smartsharesystems.com >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; >mb@smartsharesystems.com; Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; zhoumin@loongson.cn >Subject: [EXT] Re: [PATCH v5 0/4] add support for self monitoring > >External Email > >---------------------------------------------------------------------- >hi, > >don't interpret this as an objection to the functionality but this looks like a clear example of >something that doesn't belong in the EAL. has there been a discussion as to whether or not this >should be in a separate library? No, I don't recall anybody having any concerns about the code placement. Rationale behind making this part of eal was based on the fact that tracing itself is a part of eal and since this was meant to be extension to tracing, code placement decision came out naturally. During development phase idea evolved a bit and what initially was supposed to be solely yet another tracepoint become generic API to read pmu and tracepoint based on that. Which means both can be used independently. That said, since this code has both platform agnostic and platform specific parts this can either be split into: 1. library + eal platform code 2. all under eal Either approach seems legit. Thoughts? > >a basic test is whether or not an implementation exists or can be reasonably provided for all >platforms and that isn't strictly evident here. red flag is to see yet more code being added >conditionally compiled for a single platform. Even libs are not entirely pristine and have platform specific ifdefs lurking so not sure where this red flag is coming from. > >Morten, Bruce comments? > >thanks > >On Wed, Jan 11, 2023 at 12:46:38AM +0100, Tomasz Duszynski wrote: >> This series adds self monitoring support i.e allows to configure and >> read performance measurement unit (PMU) counters in runtime without >> using perf utility. This has certain adventages when application runs >> on isolated cores with nohz_full kernel parameter. >> >> Events can be read directly using rte_pmu_read() or using dedicated >> tracepoint rte_eal_trace_pmu_read(). The latter will cause events to >> be stored inside CTF file. >> >> By design, all enabled events are grouped together and the same group >> is attached to lcores that use self monitoring funtionality. >> >> Events are enabled by names, which need to be read from standard >> location under sysfs i.e >> >> /sys/bus/event_source/devices/PMU/events >> >> where PMU is a core pmu i.e one measuring cpu events. As of today raw >> events are not supported. >> >> v5: >> - address review comments >> - fix sign extension while reading pmu on x86 >> - fix regex mentioned in doc >> - various minor changes/improvements here and there >> v4: >> - fix freeing mem detected by debug_autotest >> v3: >> - fix shared build >> v2: >> - fix problems reported by test build infra >> >> Tomasz Duszynski (4): >> eal: add generic support for reading PMU events >> eal/arm: support reading ARM PMU events in runtime >> eal/x86: support reading Intel PMU events in runtime >> eal: add PMU support to tracing library >> >> app/test/meson.build | 1 + >> app/test/test_pmu.c | 47 +++ >> app/test/test_trace_perf.c | 4 + >> doc/guides/prog_guide/profile_app.rst | 13 + >> doc/guides/prog_guide/trace_lib.rst | 32 ++ >> lib/eal/arm/include/meson.build | 1 + >> lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ >> lib/eal/arm/meson.build | 4 + >> lib/eal/arm/rte_pmu.c | 104 +++++ >> lib/eal/common/eal_common_trace_points.c | 3 + >> lib/eal/common/meson.build | 3 + >> lib/eal/common/pmu_private.h | 41 ++ >> lib/eal/common/rte_pmu.c | 504 +++++++++++++++++++++++ >> lib/eal/include/meson.build | 1 + >> lib/eal/include/rte_eal_trace.h | 10 + >> lib/eal/include/rte_pmu.h | 202 +++++++++ >> lib/eal/linux/eal.c | 4 + >> lib/eal/version.map | 7 + >> lib/eal/x86/include/meson.build | 1 + >> lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ >> 20 files changed, 1054 insertions(+) >> create mode 100644 app/test/test_pmu.c create mode 100644 >> lib/eal/arm/include/rte_pmu_pmc.h create mode 100644 >> lib/eal/arm/rte_pmu.c create mode 100644 lib/eal/common/pmu_private.h >> create mode 100644 lib/eal/common/rte_pmu.c create mode 100644 >> lib/eal/include/rte_pmu.h create mode 100644 >> lib/eal/x86/include/rte_pmu_pmc.h >> >> -- >> 2.34.1
>-----Original Message----- >From: Morten Brørup <mb@smartsharesystems.com> >Sent: Wednesday, January 11, 2023 10:31 AM >To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Tomasz Duszynski <tduszynski@marvell.com>; >bruce.richardson@intel.com >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; >Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; zhoumin@loongson.cn >Subject: [EXT] RE: [PATCH v5 0/4] add support for self monitoring > >External Email > >---------------------------------------------------------------------- >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] >> Sent: Wednesday, 11 January 2023 01.32 >> >> On Wed, Jan 11, 2023 at 12:46:38AM +0100, Tomasz Duszynski wrote: >> > This series adds self monitoring support i.e allows to configure and >> > read performance measurement unit (PMU) counters in runtime without >> > using perf utility. This has certain adventages when application >> > runs >> on >> > isolated cores with nohz_full kernel parameter. >> > >> > Events can be read directly using rte_pmu_read() or using dedicated >> > tracepoint rte_eal_trace_pmu_read(). The latter will cause events to >> be >> > stored inside CTF file. >> > >> > By design, all enabled events are grouped together and the same >> > group is attached to lcores that use self monitoring funtionality. >> > >> > Events are enabled by names, which need to be read from standard >> > location under sysfs i.e >> > >> > /sys/bus/event_source/devices/PMU/events >> > >> > where PMU is a core pmu i.e one measuring cpu events. As of today >> > raw events are not supported. >> > >> > v5: >> > - address review comments >> > - fix sign extension while reading pmu on x86 >> > - fix regex mentioned in doc >> > - various minor changes/improvements here and there >> > v4: >> > - fix freeing mem detected by debug_autotest >> > v3: >> > - fix shared build >> > v2: >> > - fix problems reported by test build infra >> > >> > Tomasz Duszynski (4): >> > eal: add generic support for reading PMU events >> > eal/arm: support reading ARM PMU events in runtime >> > eal/x86: support reading Intel PMU events in runtime >> > eal: add PMU support to tracing library >> > >> > app/test/meson.build | 1 + >> > app/test/test_pmu.c | 47 +++ >> > app/test/test_trace_perf.c | 4 + >> > doc/guides/prog_guide/profile_app.rst | 13 + >> > doc/guides/prog_guide/trace_lib.rst | 32 ++ >> > lib/eal/arm/include/meson.build | 1 + >> > lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ >> > lib/eal/arm/meson.build | 4 + >> > lib/eal/arm/rte_pmu.c | 104 +++++ >> > lib/eal/common/eal_common_trace_points.c | 3 + >> > lib/eal/common/meson.build | 3 + >> > lib/eal/common/pmu_private.h | 41 ++ >> > lib/eal/common/rte_pmu.c | 504 >> +++++++++++++++++++++++ >> > lib/eal/include/meson.build | 1 + >> > lib/eal/include/rte_eal_trace.h | 10 + >> > lib/eal/include/rte_pmu.h | 202 +++++++++ >> > lib/eal/linux/eal.c | 4 + >> > lib/eal/version.map | 7 + >> > lib/eal/x86/include/meson.build | 1 + >> > lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ >> > 20 files changed, 1054 insertions(+) create mode 100644 >> > app/test/test_pmu.c create mode 100644 >> > lib/eal/arm/include/rte_pmu_pmc.h create mode 100644 >> > lib/eal/arm/rte_pmu.c create mode 100644 >> > lib/eal/common/pmu_private.h create mode 100644 >> > lib/eal/common/rte_pmu.c create mode 100644 >> > lib/eal/include/rte_pmu.h create mode 100644 >> > lib/eal/x86/include/rte_pmu_pmc.h >> > >> > -- >> > 2.34.1 > >[Moved Tyler's post down here.] > >> >> hi, >> >> don't interpret this as an objection to the functionality but this >> looks like a clear example of something that doesn't belong in the >> EAL. has there been a discussion as to whether or not this should be >> in a separate library? > >IIRC, there has been no such discussion. > >Although I agree that this doesn't belong in EAL, I would point to the trace library as a reference >for allowing it into the EAL. > >For the records, I also oppose to the trace library being part of the EAL. > >On the other hand, it would be interesting to determine if it is *impossible* adding this >functionality as any other normal DPDK library, i.e. outside of the EAL, or if there is an >unavoidable tie-in to the EAL. > >@Tomasz, if this is impossible, please describe the unavoidable tie-in to the EAL. No need for a >long report, just a few words. You (and this functionality) shouldn't suffer from our long term >ambition to move stuff out of the EAL. > You can read about rationale here https://lore.kernel.org/dpdk-dev/DM4PR18MB436872EBC5922084C5DAFC1DD2FC9@DM4PR18MB4368.namprd18.prod.outlook.com/#t As for the NO-NO there isn't any in fact. There are some tradeoffs though. For example, seems eal cannot depend on other libs so if someone needs to finetune some part of EAL for whatever reason, then relevant part needs to modified each and every time. I.e specific includes and trcepoints need to be added each time. On the other hand, if this is coupled with eal then adding tracepoints to some parts will be easier. Or they can just be added to specific points and live there. No strong opinions besides that. I'd like to know what others think. >> >> a basic test is whether or not an implementation exists or can be >> reasonably provided for all platforms and that isn't strictly evident >> here. red flag is to see yet more code being added conditionally >> compiled for a single platform. > >Another basic test: Can DPDK applications run without it? If they can, an Environment Abstraction >Layer does not need to have it, and thus it does not need to be part of the EAL. > >> >> Morten, Bruce comments? >> >> thanks
On Wed, Jan 11, 2023 at 02:24:28PM +0000, Tomasz Duszynski wrote: > > > >-----Original Message----- > >From: Morten Brørup <mb@smartsharesystems.com> > >Sent: Wednesday, January 11, 2023 10:31 AM > >To: Tyler Retzlaff <roretzla@linux.microsoft.com>; Tomasz Duszynski <tduszynski@marvell.com>; > >bruce.richardson@intel.com > >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; > >Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; zhoumin@loongson.cn > >Subject: [EXT] RE: [PATCH v5 0/4] add support for self monitoring > > > >External Email > > > >---------------------------------------------------------------------- > >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > >> Sent: Wednesday, 11 January 2023 01.32 > >> > >> On Wed, Jan 11, 2023 at 12:46:38AM +0100, Tomasz Duszynski wrote: > >> > This series adds self monitoring support i.e allows to configure and > >> > read performance measurement unit (PMU) counters in runtime without > >> > using perf utility. This has certain adventages when application > >> > runs > >> on > >> > isolated cores with nohz_full kernel parameter. > >> > > >> > Events can be read directly using rte_pmu_read() or using dedicated > >> > tracepoint rte_eal_trace_pmu_read(). The latter will cause events to > >> be > >> > stored inside CTF file. > >> > > >> > By design, all enabled events are grouped together and the same > >> > group is attached to lcores that use self monitoring funtionality. > >> > > >> > Events are enabled by names, which need to be read from standard > >> > location under sysfs i.e > >> > > >> > /sys/bus/event_source/devices/PMU/events > >> > > >> > where PMU is a core pmu i.e one measuring cpu events. As of today > >> > raw events are not supported. > >> > > >> > v5: > >> > - address review comments > >> > - fix sign extension while reading pmu on x86 > >> > - fix regex mentioned in doc > >> > - various minor changes/improvements here and there > >> > v4: > >> > - fix freeing mem detected by debug_autotest > >> > v3: > >> > - fix shared build > >> > v2: > >> > - fix problems reported by test build infra > >> > > >> > Tomasz Duszynski (4): > >> > eal: add generic support for reading PMU events > >> > eal/arm: support reading ARM PMU events in runtime > >> > eal/x86: support reading Intel PMU events in runtime > >> > eal: add PMU support to tracing library > >> > > >> > app/test/meson.build | 1 + > >> > app/test/test_pmu.c | 47 +++ > >> > app/test/test_trace_perf.c | 4 + > >> > doc/guides/prog_guide/profile_app.rst | 13 + > >> > doc/guides/prog_guide/trace_lib.rst | 32 ++ > >> > lib/eal/arm/include/meson.build | 1 + > >> > lib/eal/arm/include/rte_pmu_pmc.h | 39 ++ > >> > lib/eal/arm/meson.build | 4 + > >> > lib/eal/arm/rte_pmu.c | 104 +++++ > >> > lib/eal/common/eal_common_trace_points.c | 3 + > >> > lib/eal/common/meson.build | 3 + > >> > lib/eal/common/pmu_private.h | 41 ++ > >> > lib/eal/common/rte_pmu.c | 504 > >> +++++++++++++++++++++++ > >> > lib/eal/include/meson.build | 1 + > >> > lib/eal/include/rte_eal_trace.h | 10 + > >> > lib/eal/include/rte_pmu.h | 202 +++++++++ > >> > lib/eal/linux/eal.c | 4 + > >> > lib/eal/version.map | 7 + > >> > lib/eal/x86/include/meson.build | 1 + > >> > lib/eal/x86/include/rte_pmu_pmc.h | 33 ++ > >> > 20 files changed, 1054 insertions(+) create mode 100644 > >> > app/test/test_pmu.c create mode 100644 > >> > lib/eal/arm/include/rte_pmu_pmc.h create mode 100644 > >> > lib/eal/arm/rte_pmu.c create mode 100644 > >> > lib/eal/common/pmu_private.h create mode 100644 > >> > lib/eal/common/rte_pmu.c create mode 100644 > >> > lib/eal/include/rte_pmu.h create mode 100644 > >> > lib/eal/x86/include/rte_pmu_pmc.h > >> > > >> > -- > >> > 2.34.1 > > > >[Moved Tyler's post down here.] > > > >> > >> hi, > >> > >> don't interpret this as an objection to the functionality but this > >> looks like a clear example of something that doesn't belong in the > >> EAL. has there been a discussion as to whether or not this should be > >> in a separate library? > > > >IIRC, there has been no such discussion. > > > >Although I agree that this doesn't belong in EAL, I would point to the trace library as a reference > >for allowing it into the EAL. > > > >For the records, I also oppose to the trace library being part of the EAL. > > > >On the other hand, it would be interesting to determine if it is *impossible* adding this > >functionality as any other normal DPDK library, i.e. outside of the EAL, or if there is an > >unavoidable tie-in to the EAL. > > > >@Tomasz, if this is impossible, please describe the unavoidable tie-in to the EAL. No need for a > >long report, just a few words. You (and this functionality) shouldn't suffer from our long term > >ambition to move stuff out of the EAL. > > > > You can read about rationale here https://lore.kernel.org/dpdk-dev/DM4PR18MB436872EBC5922084C5DAFC1DD2FC9@DM4PR18MB4368.namprd18.prod.outlook.com/#t > > As for the NO-NO there isn't any in fact. There are some tradeoffs though. > > For example, seems eal cannot depend on other libs so if someone needs to > finetune some part of EAL for whatever reason, then relevant part needs to > modified each and every time. I.e specific includes and trcepoints need to be added each time. > Well, EAL can depend on other libs, but then those libs cannot in turn directly depend upon DPDK. This is where breaking out first some of the smaller widely used parts of DPDK e.g. logging, would be good, as it would then in turn allow other, potentially bigger parts of EAL to be taken out. See [1] for a rough first attempt at this, which allows simlification of telemetry as it no longer needs a "dependency injection" style to have logging. Moving out logging would also allow logging from kvargs library too - another lib which is used by EAL rather than depending on it. Similarly for tracing functionality - if that were pulled out of EAL, it could be used by telemetry, kvargs and any other parts removed from EAL. /Bruce [1] http://patches.dpdk.org/project/dpdk/list/?series=24453
On Wed, Jan 11, 2023 at 09:39:35AM +0000, Tomasz Duszynski wrote: > Hi Tyler, > > >-----Original Message----- > >From: Tyler Retzlaff <roretzla@linux.microsoft.com> > >Sent: Wednesday, January 11, 2023 1:32 AM > >To: Tomasz Duszynski <tduszynski@marvell.com>; bruce.richardson@intel.com; mb@smartsharesystems.com > >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; > >mb@smartsharesystems.com; Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; zhoumin@loongson.cn > >Subject: [EXT] Re: [PATCH v5 0/4] add support for self monitoring > > > >External Email > > > >---------------------------------------------------------------------- > >hi, > > > >don't interpret this as an objection to the functionality but this looks like a clear example of > >something that doesn't belong in the EAL. has there been a discussion as to whether or not this > >should be in a separate library? > > No, I don't recall anybody having any concerns about the code placement. Rationale behind > making this part of eal was based on the fact that tracing itself is a part of eal and > since this was meant to be extension to tracing, code placement decision came out naturally. > > During development phase idea evolved a bit and what initially was supposed to be solely yet > another tracepoint become generic API to read pmu and tracepoint based on that. Which means > both can be used independently. > > That said, since this code has both platform agnostic and platform specific parts this can either be split into: > 1. library + eal platform code > 2. all under eal > > Either approach seems legit. Thoughts? > > > > >a basic test is whether or not an implementation exists or can be reasonably provided for all > >platforms and that isn't strictly evident here. red flag is to see yet more code being added > >conditionally compiled for a single platform. > > Even libs are not entirely pristine and have platform specific ifdefs lurking so not sure where > this red flag is coming from. i think red flag was probably the wrong term to use sorry for that. rather i should say it is an indicator that the api probably doesn't belong in the eal. fundamentally the purpose of the abstraction library is to relieve the application from having to do conditional compilation and/or execution for the subject apis coming from eal. including and exporting apis that work for only one platform is in direct contradiction. please explore adding this as a separate library, it is understood that there are tradeoffs involved. thanks!
>-----Original Message----- >From: Tyler Retzlaff <roretzla@linux.microsoft.com> >Sent: Wednesday, January 11, 2023 10:06 PM >To: Tomasz Duszynski <tduszynski@marvell.com> >Cc: bruce.richardson@intel.com; mb@smartsharesystems.com; dev@dpdk.org; thomas@monjalon.net; Jerin >Jacob Kollanukkaran <jerinj@marvell.com>; Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; >zhoumin@loongson.cn >Subject: Re: [EXT] Re: [PATCH v5 0/4] add support for self monitoring > >On Wed, Jan 11, 2023 at 09:39:35AM +0000, Tomasz Duszynski wrote: >> Hi Tyler, >> >> >-----Original Message----- >> >From: Tyler Retzlaff <roretzla@linux.microsoft.com> >> >Sent: Wednesday, January 11, 2023 1:32 AM >> >To: Tomasz Duszynski <tduszynski@marvell.com>; >> >bruce.richardson@intel.com; mb@smartsharesystems.com >> >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran >> ><jerinj@marvell.com>; mb@smartsharesystems.com; Ruifeng.Wang@arm.com; >> >mattias.ronnblom@ericsson.com; zhoumin@loongson.cn >> >Subject: [EXT] Re: [PATCH v5 0/4] add support for self monitoring >> > >> >External Email >> > >> >--------------------------------------------------------------------- >> >- >> >hi, >> > >> >don't interpret this as an objection to the functionality but this >> >looks like a clear example of something that doesn't belong in the >> >EAL. has there been a discussion as to whether or not this should be in a separate library? >> >> No, I don't recall anybody having any concerns about the code >> placement. Rationale behind making this part of eal was based on the >> fact that tracing itself is a part of eal and since this was meant to be extension to tracing, >code placement decision came out naturally. >> >> During development phase idea evolved a bit and what initially was >> supposed to be solely yet another tracepoint become generic API to >> read pmu and tracepoint based on that. Which means both can be used independently. >> >> That said, since this code has both platform agnostic and platform specific parts this can either >be split into: >> 1. library + eal platform code >> 2. all under eal >> >> Either approach seems legit. Thoughts? >> >> > >> >a basic test is whether or not an implementation exists or can be >> >reasonably provided for all platforms and that isn't strictly evident >> >here. red flag is to see yet more code being added conditionally compiled for a single platform. >> >> Even libs are not entirely pristine and have platform specific ifdefs >> lurking so not sure where this red flag is coming from. > >i think red flag was probably the wrong term to use sorry for that. >rather i should say it is an indicator that the api probably doesn't belong in the eal. > >fundamentally the purpose of the abstraction library is to relieve the application from having to >do conditional compilation and/or execution for the subject apis coming from eal. including and >exporting apis that work for only one platform is in direct contradiction. > >please explore adding this as a separate library, it is understood that there are tradeoffs >involved. > >thanks! Any ideas how to name the library?
On Fri, Jan 13, 2023 at 07:44:57AM +0000, Tomasz Duszynski wrote: > > > >-----Original Message----- > >From: Tyler Retzlaff <roretzla@linux.microsoft.com> > >Sent: Wednesday, January 11, 2023 10:06 PM > >To: Tomasz Duszynski <tduszynski@marvell.com> > >Cc: bruce.richardson@intel.com; mb@smartsharesystems.com; dev@dpdk.org; thomas@monjalon.net; Jerin > >Jacob Kollanukkaran <jerinj@marvell.com>; Ruifeng.Wang@arm.com; mattias.ronnblom@ericsson.com; > >zhoumin@loongson.cn > >Subject: Re: [EXT] Re: [PATCH v5 0/4] add support for self monitoring > > > >On Wed, Jan 11, 2023 at 09:39:35AM +0000, Tomasz Duszynski wrote: > >> Hi Tyler, > >> > >> >-----Original Message----- > >> >From: Tyler Retzlaff <roretzla@linux.microsoft.com> > >> >Sent: Wednesday, January 11, 2023 1:32 AM > >> >To: Tomasz Duszynski <tduszynski@marvell.com>; > >> >bruce.richardson@intel.com; mb@smartsharesystems.com > >> >Cc: dev@dpdk.org; thomas@monjalon.net; Jerin Jacob Kollanukkaran > >> ><jerinj@marvell.com>; mb@smartsharesystems.com; Ruifeng.Wang@arm.com; > >> >mattias.ronnblom@ericsson.com; zhoumin@loongson.cn > >> >Subject: [EXT] Re: [PATCH v5 0/4] add support for self monitoring > >> > > >> >External Email > >> > > >> >--------------------------------------------------------------------- > >> >- > >> >hi, > >> > > >> >don't interpret this as an objection to the functionality but this > >> >looks like a clear example of something that doesn't belong in the > >> >EAL. has there been a discussion as to whether or not this should be in a separate library? > >> > >> No, I don't recall anybody having any concerns about the code > >> placement. Rationale behind making this part of eal was based on the > >> fact that tracing itself is a part of eal and since this was meant to be extension to tracing, > >code placement decision came out naturally. > >> > >> During development phase idea evolved a bit and what initially was > >> supposed to be solely yet another tracepoint become generic API to > >> read pmu and tracepoint based on that. Which means both can be used independently. > >> > >> That said, since this code has both platform agnostic and platform specific parts this can either > >be split into: > >> 1. library + eal platform code > >> 2. all under eal > >> > >> Either approach seems legit. Thoughts? > >> > >> > > >> >a basic test is whether or not an implementation exists or can be > >> >reasonably provided for all platforms and that isn't strictly evident > >> >here. red flag is to see yet more code being added conditionally compiled for a single platform. > >> > >> Even libs are not entirely pristine and have platform specific ifdefs > >> lurking so not sure where this red flag is coming from. > > > >i think red flag was probably the wrong term to use sorry for that. > >rather i should say it is an indicator that the api probably doesn't belong in the eal. > > > >fundamentally the purpose of the abstraction library is to relieve the application from having to > >do conditional compilation and/or execution for the subject apis coming from eal. including and > >exporting apis that work for only one platform is in direct contradiction. > > > >please explore adding this as a separate library, it is understood that there are tradeoffs > >involved. > > > >thanks! > > Any ideas how to name the library? naming is always so hard and i'm definitely not authoritative. it seems like lib/pmu would be the least churn against your existing patch series, here are some other suggestions that might work. lib/pmu (measuring unit) lib/pmc (measuring counters) lib/pcq (counter query) lib/pmq (measuring query)
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Friday, 13 January 2023 20.22 > > On Fri, Jan 13, 2023 at 07:44:57AM +0000, Tomasz Duszynski wrote: > > > > >From: Tyler Retzlaff <roretzla@linux.microsoft.com> > > >Sent: Wednesday, January 11, 2023 10:06 PM > > > > > >On Wed, Jan 11, 2023 at 09:39:35AM +0000, Tomasz Duszynski wrote: > > >> Hi Tyler, > > >> > > >> >From: Tyler Retzlaff <roretzla@linux.microsoft.com> > > >> >Sent: Wednesday, January 11, 2023 1:32 AM > > >> > > > >> >hi, > > >> > > > >> >don't interpret this as an objection to the functionality but > this > > >> >looks like a clear example of something that doesn't belong in > the > > >> >EAL. has there been a discussion as to whether or not this should > be in a separate library? > > >> > > >> No, I don't recall anybody having any concerns about the code > > >> placement. Rationale behind making this part of eal was based on > the > > >> fact that tracing itself is a part of eal and since this was meant > to be extension to tracing, > > >code placement decision came out naturally. > > >> > > >> During development phase idea evolved a bit and what initially was > > >> supposed to be solely yet another tracepoint become generic API to > > >> read pmu and tracepoint based on that. Which means both can be > used independently. > > >> > > >> That said, since this code has both platform agnostic and platform > specific parts this can either > > >be split into: > > >> 1. library + eal platform code > > >> 2. all under eal > > >> > > >> Either approach seems legit. Thoughts? > > >> > > >> > > > >> >a basic test is whether or not an implementation exists or can be > > >> >reasonably provided for all platforms and that isn't strictly > evident > > >> >here. red flag is to see yet more code being added conditionally > compiled for a single platform. > > >> > > >> Even libs are not entirely pristine and have platform specific > ifdefs > > >> lurking so not sure where this red flag is coming from. > > > > > >i think red flag was probably the wrong term to use sorry for that. > > >rather i should say it is an indicator that the api probably doesn't > belong in the eal. > > > > > >fundamentally the purpose of the abstraction library is to relieve > the application from having to > > >do conditional compilation and/or execution for the subject apis > coming from eal. including and > > >exporting apis that work for only one platform is in direct > contradiction. > > > > > >please explore adding this as a separate library, it is understood > that there are tradeoffs > > >involved. > > > > > >thanks! > > > > Any ideas how to name the library? > > naming is always so hard and i'm definitely not authoritative. > > it seems like lib/pmu would be the least churn against your existing > patch series, here are some other suggestions that might work. +1 to lib/pmu Less work, as Tyler already mentioned. Furthermore: Both Intel and ARM use the term Performance Monitoring Unit (abbreviated PMU). Microsoft does too [1]. [1]: https://learn.microsoft.com/en-us/windows-hardware/test/wpt/recording-pmu-events RISC-V uses the term Hardware Performance Monitor (abbreviated HPM). I haven't checked other CPU vendors.
PMU is kind of MSR. Precisely , that's MSR per core. All MSR reading will lead to IPI(Please reference the kernel implementation of MSR driver). The IPI will disturb the DPDK application because the userspace/kernel context switch, which has impact to the tail latency.