RE: [EXTERNAL] Re: [PATCH v13 4/4] eal: add PMU support to tracing library

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Tomasz Duszynski [mailto:tduszynski@marvell.com]
> Sent: Thursday, 10 October 2024 09.24
> 
> >On Wed, 9 Oct 2024 14:50:02 +0200
> >Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> >> > From: Tomasz Duszynski [mailto:tduszynski@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(&reg, "e=([_[:alnum:]-],?)+", REG_EXTENDED);
> >> > +	if (ret)
> >> > +		return -EINVAL;
> >> > +
> >> > +	for (;;) {
> >> > +		if (regexec(&reg, 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(&reg);
> >> > +
> >> > +	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. ;-)