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 02449A0542; Fri, 16 Dec 2022 08:33:38 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 98E5B40695; Fri, 16 Dec 2022 08:33:38 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id A6A8540685 for ; Fri, 16 Dec 2022 08:33:37 +0100 (CET) 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: [PATCH v4 1/4] eal: add generic support for reading PMU events Date: Fri, 16 Dec 2022 08:33:33 +0100 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D875B2@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D875A6@smartserver.smartshare.dk> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v4 1/4] eal: add generic support for reading PMU events Thread-Index: AQHZDt/QEhRtFHSga0yHSOMIy5Zzs65rtOuAgAFVOCCAABlQ4IABed8QgAGFRnA= References: <20221129092821.1304853-1-tduszynski@marvell.com> <20221213104350.3218167-1-tduszynski@marvell.com> <20221213104350.3218167-2-tduszynski@marvell.com> <98CBD80474FA8B44BF855DF32C47DC35D8758C@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D8759B@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D875A6@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tomasz Duszynski" , Cc: , "Jerin Jacob Kollanukkaran" , , , 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: Morten Br=F8rup [mailto:mb@smartsharesystems.com] > Sent: Thursday, 15 December 2022 09.22 >=20 > > From: Morten Br=F8rup [mailto:mb@smartsharesystems.com] > > Sent: Wednesday, 14 December 2022 11.41 > > > > +CC: Mattias, see my comment below about per-thread constructor for > > this > > > > > From: Tomasz Duszynski [mailto:tduszynski@marvell.com] > > > Sent: Wednesday, 14 December 2022 10.39 > > > > > > Hello Morten, > > > > > > Thanks for review. Answers inline. > > > > > > [...] > > > > > > > > +__rte_experimental > > > > > +static __rte_always_inline uint64_t > > > > > +rte_pmu_read(int index) > > > > The index type can be changed from int to uint32_t. This also > > eliminates the "(index < 0" part of the comparison further below in > > this function. > > > > > > > +{ > > > > > + int lcore_id =3D rte_lcore_id(); > > > > > + struct rte_pmu_event_group *group; > > > > > + int ret; > > > > > + > > > > > + if (!rte_pmu) > > > > > + return 0; > > > > > + > > > > > + group =3D &rte_pmu->group[lcore_id]; > > > > > + if (!group->enabled) { > > > > Optimized: if (unlikely(!group->enabled)) { > > > > > > > + ret =3D rte_pmu_enable_group(lcore_id); > > > > > + if (ret) > > > > > + return 0; > > > > > + > > > > > + group->enabled =3D true; > > > > > + } > > > > > > > > Why is the group not enabled in the setup function, > > > rte_pmu_add_event(), instead of here, in the > > > > hot path? > > > > > > > > > > When this is executed for the very first time then cpu will have > > > obviously more work to do > > > but afterwards setup path is not taken hence much less cpu cycles > are > > > required. > > > > > > Setup is executed by main lcore solely, before lcores are executed > > > hence some info passed to > > > SYS_perf_event_open ioctl() is missing, pid (via rte_gettid()) > being > > an > > > example here. > > > > OK. Thank you for the explanation. Since impossible at setup, it has > to > > be done at runtime. > > > > @Mattias: Another good example of something that would belong in = per- > > thread constructors, as my suggested feature creep in [1]. > > > > [1]: > > > = http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D87553@smarts > > erver.smartshare.dk/ >=20 > I just realized that this initialization is per-lcore (not per = thread), > so you can use rte_lcore_callback_register() to register a per-lcore > initialization function, and move rte_pmu_enable_group(lcore_id) = there. Sorry, Thomasz! You can't use rte_lcore_callback_register()... it doesn't provide = per-lcore thread constructors/destructors the way I thought. :-(