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

[Tomasz Duszynski <tduszynski@marvell.com>]

>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.