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

[Morten Brørup]

> 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]: http://inbox.dpdk.org/dev/DM4PR18MB4368461EC42603F77A7DC1BCD2E09@DM4PR18MB4368.namprd18.prod.outlook.com/

> >
> 
> 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().

[2]: https://elixir.bootlin.com/dpdk/latest/source/lib/mempool/rte_mempool.h#L1315

> 
> 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.
> >
>