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 4BA3C4248B; Thu, 26 Jan 2023 13:29:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3EC5A42DC0; Thu, 26 Jan 2023 13:29:39 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 1A6FA40A79 for ; Thu, 26 Jan 2023 13:29:38 +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: [PATCH v6 1/4] lib: add generic support for reading PMU events Date: Thu, 26 Jan 2023 13:29:36 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D876B6@smartserver.smartshare.dk> In-Reply-To: A X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v6 1/4] lib: add generic support for reading PMU events Thread-Index: AQHZLF9HxL6yMLPInESeqtNvHMF+j66nD34AgAlimuCAACzbsA== References: <20230110234642.1188550-1-tduszynski@marvell.com> <20230119233916.4029128-1-tduszynski@marvell.com> <20230119233916.4029128-2-tduszynski@marvell.com> <98CBD80474FA8B44BF855DF32C47DC35D8769A@smartserver.smartshare.dk> A From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tomasz Duszynski" , , "Thomas Monjalon" 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: Tomasz Duszynski [mailto:tduszynski@marvell.com] > Sent: Thursday, 26 January 2023 10.40 >=20 > >From: Morten Br=F8rup > >Sent: Friday, January 20, 2023 10:47 AM > > > >> From: Tomasz Duszynski [mailto:tduszynski@marvell.com] > >> Sent: Friday, 20 January 2023 00.39 > >> > >> Add support for programming PMU counters and reading their values = in > >> runtime bypassing kernel completely. > >> > >> This is especially useful in cases where CPU cores are isolated > >> (nohz_full) i.e run dedicated tasks. In such cases one cannot use > >> standard perf utility without sacrificing latency and performance. > >> > >> Signed-off-by: Tomasz Duszynski > >> --- > > > >If you insist on passing lcore_id around as a function parameter, the > function description must > >mention that the lcore_id parameter must be set to rte_lcore_id() for > the functions where this is a > >requirement, including all functions that use those functions. Perhaps I'm stating this wrong, so let me try to rephrase: As I understand it, some of the setup functions must be called from the = EAL thread that executes that function - due to some syscall = (SYS_perf_event_open) needing to be called from the thread itself. Those functions should not take an lcore_id parameter. Otherwise, I = would expect to be able to call those functions from e.g. the main = thread and pass the lcore_id of any EAL thread as a parameter, which you = at the bottom of this email [1] explained is not possible. [1]: = http://inbox.dpdk.org/dev/DM4PR18MB4368461EC42603F77A7DC1BCD2E09@DM4PR18M= B4368.namprd18.prod.outlook.com/ > > >=20 > Not sure why are you insisting so much on removing that = rte_lcore_id(). > Yes that macro evaluates > to integer but if you don't think about internals this resembles a > function call. I agree with this argument. And for that reason, passing lcore_id around = could be relevant. I only wanted to bring your attention to the low cost of fetching it = inside the functions, as an alternative to passing it as an argument. >=20 > Then natural pattern is to call it once and reuse results if possible. Yes, and I would usually agree to using this pattern. > Passing lcore_id around > implies that calls are per l-core, why would that confuse anyone > reading that code? This is where I disagree: Passing lcore_id as a parameter to a function = does NOT imply that the function is running on that lcore! E.g rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id) = [2] takes lcore_id as a parameter, and does not assume that = lcore_id=3D=3Drte_lcore_id(). [2]: = https://elixir.bootlin.com/dpdk/latest/source/lib/mempool/rte_mempool.h#L= 1315 >=20 > Besides, all functions taking it are internal stuff hence you cannot > call it elsewhere. OK. I agree that this reduces the risk of incorrect use. Generally, I think that internal functions should be documented too. Not = to the full extent, like public functions, but some documentation is = nice. And if there are special requirements to a function parameter, it should = be documented with that function. Requiring that the lcore_id parameter = must be =3D=3D rte_lcore_id() is certainly a special requirement. It might just be me worrying too much, so... If nobody else complains = about this, I can live with it as is. Assuming that none of the public = functions have this special requirement (either directly or indirectly, = by calling functions with the special requirement). >=20 > >Alternatively, follow my previous suggestion: Omit the lcore_id > function parameter, and use > >rte_lcore_id() instead. > > >=20