On Thu, Jan 26, 2023 at 01:29:36PM +0100, Morten Brørup wrote: > > 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]: > http://inbox.dpdk.org/dev/dm4pr18mb4368461ec42603f77a7dc1bcd2...@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). > I would tend to agree with you Morten. If the lcore_id parameter to the function must be rte_lcore_id(), then I think it's error prone to have that as an explicit parameter, and that the function should always get the core id itself.
Other possible complication is - how does this work with threads that are not pinned to a particular physical core? Do things work as expected in that case? /Bruce
