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

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>
---
 app/test/test_trace_perf.c               |  4 ++
 doc/guides/prog_guide/profile_app.rst    |  5 ++
 doc/guides/prog_guide/trace_lib.rst      | 32 +++++++++++++
 lib/eal/common/eal_common_trace.c        |  7 ++-
 lib/eal/common/eal_common_trace_points.c |  5 ++
 lib/eal/include/rte_eal_trace.h          |  9 ++++
 lib/eal/meson.build                      |  2 +-
 lib/eal/version.map                      |  3 ++
 lib/pmu/rte_pmu.c                        | 61 ++++++++++++++++++++++++
 lib/pmu/rte_pmu.h                        | 18 +++++++
 lib/pmu/version.map                      |  1 +
 11 files changed, 145 insertions(+), 2 deletions(-)
  

> 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
  

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.
  

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

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