Re: [PATCH v7 7/8] trace: add PMU

[Tomasz Duszynski <tduszynski@marvell.com>]

> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Monday, 21 July 2025 12.46
> >
> > 21/07/2025 12:24, Tomasz Duszynski:
> > > > On Fri, Jun 27, 2025 at 5:41 PM Tomasz Duszynski
> > <tduszynski@marvell.com> wrote:
> > > > >
> > > > > In order to profile app, one needs to store significant amount of
> > samples
> > > > > somewhere for an analysis later on.
> > > > > Since trace library supports storing data in a CTF format,
> > > > > lets take advantage of that and add a dedicated PMU tracepoint.
> > > > >
> > > > > Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
> > [...]
> > > > > --- a/lib/eal/common/eal_common_trace_points.c
> > > > > +++ b/lib/eal/common/eal_common_trace_points.c
> > > > > @@ -119,3 +119,23 @@
> > RTE_TRACE_POINT_REGISTER(rte_eal_trace_intr_enable,
> > > > >         lib.eal.intr.enable)
> > > > >  RTE_TRACE_POINT_REGISTER(rte_eal_trace_intr_disable,
> > > > >         lib.eal.intr.disable)
> > > > > +
> > > > > +#ifdef RTE_LIB_PMU
> > > > > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(__rte_pmu_trace_read, 25.07)
> > > > > +RTE_TRACE_POINT_REGISTER(rte_pmu_trace_read,
> > > > > +       lib.pmu.read)
> > > > > +#endif
> > > > > +#ifdef RTE_EXEC_ENV_IS_WINDOWS
> > > > > +/* gen-version-map.py script generates export symbol maps by
> > scanning source files without
> > > > > + * evaluating conditional compilation. Hence __rte_pmu_trace_read
> > will be included the version map
> > > > > + * even if library is not compiled.
> > > > > + *
> > > > > + * On Windows if msvc linker is used this leads to a hard link
> > error
> > > > > + * (LNK2001: unresolved external symbol) because msvc requires
> > all symbols listed in the .def file
> > > > > + * to be present in the object files.
> > > > > + *
> > > > > + * Other linkers, e.g: gnu ld or mingw ld, are more forgiving.
> > They silently ignore symbols listed
> > > > > + * in the map file if those symbols are not present in the
> > binary.
> > > > > + */
> > > > > +rte_trace_point_t __rte_pmu_trace_read;
> > > > > +#endif
> > > >
> > > > From a quick look, could you export this symbol from the PMU library
> > itself?
> > >
> > > Got caught up, but here is my take. It would likely make trace a
> > dependency, but I believe the
> > > dependency should be reversed. Also from my perspective this
> > suggestion feels more like a
> > > refactoring.
> > >
> > > So unless I've misunderstood your point, I'd rater keep the current
> > solution as is.
> >
> > I think you got it right: a refactoring looks beneficial.
> > Please think about it.
> >
>
> It's a long term goal (wishful thinking?) to move Trace out of the EAL, and into a separate library.
> Considering this long term goal, refactoring PMU to make it depend on Trace would be a step in the wrong direction.
>
> However, if we disregard this long term goal, and do consider Trace a core part of the EAL, refactoring PMU to depend on Trace seems cleaner to me too.
>
> I'm in favor of not adding more obstacles to moving Trace out of the EAL, i.e. don't refactor PMU to depend on Trace.

Tried moving that to pmu itself but unfortunately it won't compile due to current dependencies.
PMU gets build before EAL so some symbols coming from trace (which is part of EAL anyway) cannot
be resolved.

That said, I decided to cut corners and moved workaround along with lengthy explanation to
rte_pmu.h. With that, at least EAL internals are cleaner and automatic symbol export is resolved too.

Let's continue discussion under v8.

> Also, I tend to agree with Tomasz about PMU being lower level than Trace, so PMU depending on Trace seems wrong. No strong opinion on this last point, though.
>
> @Jerin, what do you think?