> From: Tomasz Duszynski [mailto:tduszyn...@marvell.com] > Sent: Thursday, 10 October 2024 09.24 > > >On Wed, 9 Oct 2024 14:50:02 +0200 > >Morten Brørup <m...@smartsharesystems.com> wrote: > > > >> > From: Tomasz Duszynski [mailto:tduszyn...@marvell.com] > >> > Sent: Wednesday, 9 October 2024 13.23 > >> > > >> > +PMU tracepoint > >> > +-------------- > >> > + > >> > +Performance monitoring unit (PMU) event values can be read from > >> > hardware > >> > +registers using predefined ``rte_pmu_read`` tracepoint. > >> > + > >> > +Tracing is enabled via ``--trace`` EAL option by passing both > >> > expression > >> > +matching PMU tracepoint name i.e ``lib.eal.pmu.read`` and > >> > +expression ``e=ev1[,ev2,...]`` matching particular events:: > >> > + > >> > + --trace='.*pmu.read\|e=cpu_cycles,l1d_cache' > >> > + > >> > +Event names are available under > >> > ``/sys/bus/event_source/devices/PMU/events`` > >> > +directory, where ``PMU`` is a placeholder for either a ``cpu`` or > a > >> > directory > >> > +containing ``cpus``. > >> > + > >> > +In contrary to other tracepoints this does not need any extra > >> > variables > >> > +added to source files. Instead, caller passes index which follows > >> > +the > >> > order of > >> > +events specified via ``--trace`` parameter. In the following > >> > +example > >> > index ``0`` > >> > +corresponds to ``cpu_cyclces`` while index ``1`` corresponds to > >> > ``l1d_cache``. > >> > + > >> > +.. code-block:: c > >> > + > >> > + ... > >> > + rte_eal_trace_pmu_read(0); > >> > + rte_eal_trace_pmu_read(1); > >> > + ... > >> > + > >> > +PMU tracing support must be explicitly enabled using the > >> > ``enable_trace_fp`` > >> > +option for meson build. > >> > + > >> > >> > >> > +int > >> > +rte_pmu_add_events_by_pattern(const char *pattern) { > >> > + regmatch_t rmatch; > >> > + char buf[BUFSIZ]; > >> > + unsigned int num; > >> > + regex_t reg; > >> > + int ret; > >> > + > >> > + /* events are matched against occurrences of e=ev1[,ev2,..] > >> > pattern */ > >> > + ret = regcomp(®, "e=([_[:alnum:]-],?)+", REG_EXTENDED); > >> > + if (ret) > >> > + return -EINVAL; > >> > + > >> > + for (;;) { > >> > + if (regexec(®, pattern, 1, &rmatch, 0)) > >> > + break; > >> > + > >> > + num = rmatch.rm_eo - rmatch.rm_so; > >> > + if (num > sizeof(buf)) > >> > + num = sizeof(buf); > >> > + > >> > + /* skip e= pattern prefix */ > >> > + memcpy(buf, pattern + rmatch.rm_so + 2, num - 2); > >> > + buf[num - 2] = '\0'; > >> > + ret = add_events(buf); > >> > + if (ret) > >> > + break; > >> > + > >> > + pattern += rmatch.rm_eo; > >> > + } > >> > + > >> > + regfree(®); > >> > + > >> > + return ret; > >> > +} > >> > >> This --trace parameter takes a regex, but the --log-level parameter > takes a globbing pattern (and > >a regex, for backwards compatibility, I assume). > >> > >> This --trace parameter should behave like the --log-level parameter, > or if not able to parse > >both, use globbing pattern, not regex. > >> > >> Check the --trace parameter parser here: > >> https://urldefense.proofpoint.com/v2/url?u=https- > 3A__elixir.bootlin.co > >> m_dpdk_v24.07_source_lib_eal_common_eal-5Fcommon-5Foptions.c-23L1409 > > Just in case, I'll stress that --trace parameter is inherited from > tracing library and that itself takes a regex. > Checked both documentation and sources for any discrepancies but there > are none. > > That means there has never been any compatibility with --log-level > behavior and I don't think that patch > should straight things out. > > Maybe addressing that separately would be more sensible. > > > > >The log level has two API's one takes regex and one takes file glob > pattern. > >The original one was the regex, but everybody (including the > documentation) got regex wrong when > >the log level format had periods in it. The glob match was more > intuitive, and that is what is > >recommended. > > > > Thanks for providing more context. > > >For tracing if you want to use regex then function should be named > with _regexp( For extra bonus > >points, having both a regex and glob version (with pattern) would be > great. > > Adding extra suffix is fine. glob match lacks some syntax used for > specifying events to read, which is \| (OR). Checked the link from > Morten > and seems regex matching is turned on whenever log-level contains a > coma. Unfortunately I treat coma as event delimiter so presumably > syntax would need to be changed. > > Maybe something like > '.*pmu.read:cpu_cycles;l1d_cache,*some_other_unrelated tp*' would be > more appropriate if making --trace similar to --log-level is the way to > go.
<rant> It's unfortunate that the parameter formats passed though various means (command line, telemetry, ...) have evolved by piling on more and more, rather than making a proper design, like the other APIs. </rant> Since this patch is related to trace, you should let trace conventions take precedence over log conventions. Please ignore my feedback about using log conventions. Sorry about the noise. ;-)
