Re: [PATCH v7] perf test pmu-events: A sub-test per metric table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch relies on undefined struct fields and missing functions, causing immediate build failures. It appears to depend on prerequisite framework changes that are not present in the tree.
--

commit bc7fcf30945d5b7552eacba5428c9cebfc153def
Author: Ian Rogers <irogers@google.com>

perf test pmu-events: A sub-test per metric table

This commit breaks apart the "Parsing of PMU event table metrics" tests into
individual tests per metric table. It implements setup_pmu_events_suite() to
dynamically allocate and populate these test cases. The parsing tests are
updated to retrieve the specific table and test only that table.

> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index a997168621688..0752f0eba085b 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -903,13 +903,20 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>  	return err;
>  }
>  
> -static int test__parsing(struct test_suite *test __maybe_unused,
> -			 int subtest __maybe_unused)
> +static int test__parsing(struct test_suite *test, int subtest)
>  {
>  	int failures = 0;
> +	const struct pmu_metrics_table *table = NULL;
>  
> -	pmu_for_each_core_metric(test__parsing_callback, &failures);
> -	pmu_for_each_sys_metric(test__parsing_callback, &failures);
> +	if (test->test_cases)
> +		table = test->test_cases[subtest].priv;

[Severity: Low]
Does struct test_case have a priv member? Looking at tools/perf/tests/tests.h,
it appears priv is a member of struct test_suite but not struct test_case,
which may cause a build failure here. Are we missing a prerequisite patch?

[ ... ]

> +static int populate_metrics_tables_cb(const struct pmu_metrics_table *table, void *data)
> +{
> +	struct populate_cb_data *cb_data = data;
> +	const char *table_name = pmu_metrics_table__name(table);
> +	char *desc_real, *desc_fake;
> +
> +	if (!table_name)
> +		table_name = "unknown";
> +
> +	if (asprintf(&desc_real, "PMU metric parsing: %s", table_name) < 0)
> +		return -ENOMEM;
> +	if (asprintf(&desc_fake, "PMU metric parsing with fake PMU: %s", table_name) < 0) {
> +		free(desc_real);
> +		return -ENOMEM;
> +	}
> +
> +	cb_data->test_cases[cb_data->curr++] = (struct test_case){
> +		.name = "parsing",
> +		.desc = desc_real,
> +		.run_case = test__parsing,
> +		.priv = (void *)table,

[Severity: Low]
Similar to the above, does the priv member exist in struct test_case?

[ ... ]

> +static int setup_pmu_events_suite(struct test_suite *suite)
> +{
> +	size_t num_tables = 0;
> +	size_t num_fixed_tests = 4;
> +	size_t tests_per_table = 2;
> +	size_t total_tests;
> +	struct test_case *test_cases;
> +	size_t curr = 0;
> +	struct populate_cb_data cb_data;
> +	int ret;
> +
> +	if (suite->test_cases != pmu_events_tests)
> +		return 0;
> +
> +	ret = pmu_metrics_table__iterate_tables(count_metrics_tables_cb, &num_tables);

[Severity: Low]
Is pmu_metrics_table__iterate_tables() defined in the tree at this commit?
It looks like the patch might depend on another series that introduces this
function and pmu_metrics_table__name() above.

[ ... ]

>  struct test_suite suite__pmu_events = {
>  	.desc = "PMU JSON event tests",
>  	.test_cases = pmu_events_tests,
> +	.setup = setup_pmu_events_suite,

[Severity: Low]
Does struct test_suite have a .setup member? Looking at its definition in
tools/perf/tests/tests.h, there only seems to be desc, test_cases, and priv.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602073132.2653307-7-irogers@google.com?part=1