>-----Original Message----- >From: Morten Brørup <m...@smartsharesystems.com> >Sent: Thursday, January 26, 2023 1:30 PM >To: Tomasz Duszynski <tduszyn...@marvell.com>; dev@dpdk.org; Thomas Monjalon ><tho...@monjalon.net> >Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; ruifeng.w...@arm.com; >mattias.ronnb...@ericsson.com; zhou...@loongson.cn; bruce.richard...@intel.com; >roret...@linux.microsoft.com >Subject: [EXT] RE: [PATCH v6 1/4] lib: add generic support for reading PMU >events > >External Email > >---------------------------------------------------------------------- >> From: Tomasz Duszynski [mailto:tduszyn...@marvell.com] >> Sent: Thursday, 26 January 2023 10.40 >> >> >From: Morten Brørup <m...@smartsharesystems.com> >> >Sent: Friday, January 20, 2023 10:47 AM >> > >> >> From: Tomasz Duszynski [mailto:tduszyn...@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 <tduszyn...@marvell.com> >> >> --- >> > >> >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]: https://urldefense.proofpoint.com/v2/url?u=http- >3A__inbox.dpdk.org_dev_DM4PR18MB4368461EC42603F77A7DC1BCD2E09- >40DM4PR18MB4368.namprd18.prod.outlook.com_&d=DwIFAw&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxI >xRndyEUwWU_ad5ce22YI6Is&m=QkMcmM2epUOCdRd6xI5o3d2nQaqruy0GvOQUgbn75cLlzobEVMwLBUiGXADiuvVz&s=5K9oM8 >e7u52C_0_5xtWIKl31aXRHhJDKoQTDQp5EHWY&e= > >> > >> >> 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. > >> >> 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==rte_lcore_id(). >
Oh, now I got your point! Okay then, if this is going to cause confusion because of misleading self-documenting code I'll change that. >[2]: https://urldefense.proofpoint.com/v2/url?u=https- >3A__elixir.bootlin.com_dpdk_latest_source_lib_mempool_rte-5Fmempool.h- >23L1315&d=DwIFAw&c=nKjWec2b6R0mOyPaz7xtfQ&r=PZNXgrbjdlXxVEEGYkxIxRndyEUwWU_ad5ce22YI6Is&m=QkMcmM2ep >UOCdRd6xI5o3d2nQaqruy0GvOQUgbn75cLlzobEVMwLBUiGXADiuvVz&s=4pnL_TZcVhj476u19ybcn2Rbad6OTb3k2U- >nhFvhZ0k&e= > >> >> 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 == 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). > >> >> >Alternatively, follow my previous suggestion: Omit the lcore_id >> function parameter, and use >> >rte_lcore_id() instead. >> > >>
