[PATCH v6 1/4] lib: add generic support for reading PMU events

[Tomasz Duszynski]

>-----Original Message-----
>From: Morten Brørup <mb at smartsharesystems.com>
>Sent: Thursday, January 26, 2023 1:30 PM
>To: Tomasz Duszynski <tduszynski at marvell.com>; dev at dpdk.org; Thomas Monjalon <thomas at monjalon.net>
>Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Ruifeng.Wang at arm.com;
>mattias.ronnblom at ericsson.com; zhoumin at loongson.cn; bruce.richardson at intel.com;
>roretzla at linux.microsoft.com
>Subject: [EXT] RE: [PATCH v6 1/4] lib: add generic support for reading PMU events
>
>External Email
>
>----------------------------------------------------------------------
>> From: Tomasz Duszynski [mailto:tduszynski at marvell.com]
>> Sent: Thursday, 26 January 2023 10.40
>>
>> >From: Morten Brørup <mb at smartsharesystems.com>
>> >Sent: Friday, January 20, 2023 10:47 AM
>> >
>> >> From: Tomasz Duszynski [mailto:tduszynski at 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 <tduszynski at 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.
>> >
>>