Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[John Garry]

On 10/08/2023 22:49, Ian Rogers wrote:
> When there are multiple PMUs that differ only by suffix, by default
> just list the first one and skip all others. As the PMUs are sorted,
> the scan routine checks that the PMU names match and the numbers are
> consecutive. If "-v" is passed to "perf list" then list all PMUs.

I really think that this should be merged with the next change. I don't 
like the intermediate step of by default only printing the first PMU.

> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>   tools/perf/builtin-list.c      |  8 +++++
>   tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
>   tools/perf/util/print-events.h |  1 +
>   3 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 7fec2cca759f..8fe4ddf02c14 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>   	strbuf_release(&buf);
>   }
>   
> +static bool default_skip_duplicate_pmus(void *ps)
> +{
> +	struct print_state *print_state = ps;
> +
> +	return !print_state->long_desc;
> +}
> +
>   int cmd_list(int argc, const char **argv)
>   {
>   	int i, ret = 0;
> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
>   		.print_end = default_print_end,
>   		.print_event = default_print_event,
>   		.print_metric = default_print_metric,
> +		.skip_duplicate_pmus = default_skip_duplicate_pmus,
>   	};
>   	const char *cputype = NULL;
>   	const char *unit_name = NULL;
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 3581710667b0..5073843aca19 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>   	return NULL;
>   }
>   
> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> +{
> +	bool use_core_pmus = !pmu || pmu->is_core;
> +	int last_pmu_name_len = 0;
> +	unsigned long last_pmu_num = 0;
> +	const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> +
> +	if (!pmu) {
> +		pmu_read_sysfs(/*core_only=*/false);
> +		pmu = list_prepare_entry(pmu, &core_pmus, list);
> +	} else
> +		last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> +
> +	if (use_core_pmus) {
> +		list_for_each_entry_continue(pmu, &core_pmus, list) {
> +			unsigned long pmu_num = 0;
> +			int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> +			if (last_pmu_name_len == pmu_name_len &&
> +			    (last_pmu_num + 1 == pmu_num) &&
> +			    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> +				last_pmu_num++;
> +				continue;
> +			}
> +			return pmu;
> +		}
> +		pmu = NULL;

you assign pmu NULL

> +		pmu = list_prepare_entry(pmu, &other_pmus, list);

and then re-assign it. If list_prepare_entry() needs first arg = NULL, 
then can just use NULL explicitly?

> +	}
> +	list_for_each_entry_continue(pmu, &other_pmus, list) {
> +		unsigned long pmu_num = 0;
> +		int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> +		if (last_pmu_name_len == pmu_name_len &&
> +		    (last_pmu_num + 1 == pmu_num) &&
> +		    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> +			last_pmu_num++;
> +			continue;

Can some of this code be factored out from the previous patch? It's 
doing something similar, right?

> +		}
> +		return pmu;
> +	}
> +	return NULL;
> +}
> +
>   const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
>   {
>   	struct perf_pmu *pmu = NULL;
> @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   	int printed = 0;
>   	int len, j;
>   	struct sevent *aliases;
> +	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> +
> +	if (print_cb->skip_duplicate_pmus(print_state))
> +		scan_fn = perf_pmus__scan_skip_duplicates;
> +	else
> +		scan_fn = perf_pmus__scan;
>   
>   	pmu = NULL;
>   	len = 0;
> -	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> +	while ((pmu = scan_fn(pmu)) != NULL) {
>   		list_for_each_entry(event, &pmu->aliases, list)
>   			len++;
>   		if (pmu->selectable)
> @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   	}
>   	pmu = NULL;
>   	j = 0;
> -	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> +	while ((pmu = scan_fn(pmu)) != NULL) {
>   		bool is_cpu = pmu->is_core;
>   
>   		list_for_each_entry(event, &pmu->aliases, list) {
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> index d7fab411e75c..bf4290bef0cd 100644
> --- a/tools/perf/util/print-events.h
> +++ b/tools/perf/util/print-events.h
> @@ -26,6 +26,7 @@ struct print_callbacks {
>   			const char *expr,
>   			const char *threshold,
>   			const char *unit);
> +	bool (*skip_duplicate_pmus)(void *print_state);
>   };
>   
>   /** Print all events, the default when no options are specified. */