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

[Thomas Monjalon <thomas@monjalon.net>]

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.