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

[Stephen Hemminger <stephen@networkplumber.org>]

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://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/common/eal_common_options.c#L1409

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.

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.