Re: [PATCH v3 02/19] perf test attr: Allow tests to require a core PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] perf python: memory leak in pyrf__parse_events() on error path
--

commit 8f696c0054607fd944b69ffbf9d282950abfb382
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf test attr: Allow tests to require a core PMU

This commit introduces a 'requires' limitation for perf tests to skip
execution if a core PMU is not available. It checks for a core PMU by using
the perf python binding to parse and open the 'cycles' event.

> diff --git a/tools/perf/tests/shell/lib/attr.py b/tools/perf/tests/shell/lib/attr.py
> index 68f06df9565a0..2d7dd26a57377 100644
> --- a/tools/perf/tests/shell/lib/attr.py
> +++ b/tools/perf/tests/shell/lib/attr.py

[ ... ]

> @@ -28,6 +29,50 @@ def data_equal(a, b):
>  
>      return False
>  
> +CORE_PMU_AVAILABLE = None
> +
> +def core_pmu_available():

[ ... ]

> +def core_pmu_probe():
> +    """
> +    Ask perf itself, through its python binding, to parse and open 'cycles':
> +    a parse failure or an ENOENT means no PMU can count it, any other error
> +    means we can't tell whether there is one.
> +    """
> +    try:
> +        import perf
> +        parse_events = perf.parse_events
> +    except (ImportError, AttributeError):
> +        # Nothing to ask about it, let the expectations speak.
> +        return True
> +
> +    try:
> +        evlist = parse_events('cycles')

[Severity: Low]
Does this trigger a memory leak when parse_events() fails?

The python C wrapper pyrf__parse_events() in tools/perf/util/python.c
appears to omit the required cleanup call on its error path:

tools/perf/util/python.c:pyrf__parse_events() {
	...
	if (parse_events(evlist, input, &err)) {
		parse_events_error__print(&err, input);
		PyErr_SetFromErrno(PyExc_OSError);
		evlist__put(evlist);
		return NULL;
	}
	...
}

While this leak was not introduced by this patch, this new core_pmu_probe()
function actively relies on and triggers this error path to detect the PMU
absence.

Could we clean up the dynamically allocated error strings within the
parse_events_error structure by calling parse_events_error__exit(&err) when
parsing fails?

> +    except OSError:
> +        # No PMU that can count it, the same as the ENOENT below.
> +        return False

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016001843.71978-1-acme@kernel.org?part=2