Re: [PATCH v1 02/10] perf stat: Don't remove all grouped events when CPU maps disagree

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:03PM -0800, Ian Rogers escreveu:
> If the events in an evlist's CPU map differ then the entire group is
> removed. For example:
> 
> ```
> $ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/,cs}' -a sleep 1
> WARNING: grouped events cpus do not match, disabling group:
>   anon group { imc_free_running/data_read/, imc_free_running/data_write/, cs }
> ```
> 
> Change the behavior so that just the events not matching the leader
> are removed. So in the example above, just 'cs' will be removed.

Its a change in behaviour but a good one, I think, Jiri?

- Arnaldo
 
> Modify the warning so that it is produced once for each group, rather
> than once for the entire evlist. Shrink the scope and size of the
> warning text buffer.
> 
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/builtin-stat.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d70b1ec88594..5c12ae5efce5 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -181,14 +181,13 @@ static bool cpus_map_matched(struct evsel *a, struct evsel *b)
>  
>  static void evlist__check_cpu_maps(struct evlist *evlist)
>  {
> -	struct evsel *evsel, *pos, *leader;
> -	char buf[1024];
> +	struct evsel *evsel, *warned_leader = NULL;
>  
>  	if (evlist__has_hybrid(evlist))
>  		evlist__warn_hybrid_group(evlist);
>  
>  	evlist__for_each_entry(evlist, evsel) {
> -		leader = evsel__leader(evsel);
> +		struct evsel *leader = evsel__leader(evsel);
>  
>  		/* Check that leader matches cpus with each member. */
>  		if (leader == evsel)
> @@ -197,19 +196,26 @@ static void evlist__check_cpu_maps(struct evlist *evlist)
>  			continue;
>  
>  		/* If there's mismatch disable the group and warn user. */
> -		WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n");
> -		evsel__group_desc(leader, buf, sizeof(buf));
> -		pr_warning("  %s\n", buf);
> -
> +		if (warned_leader != leader) {
> +			char buf[200];
> +
> +			pr_warning("WARNING: grouped events cpus do not match.\n"
> +				"Events with CPUs not matching the leader will "
> +				"be removed from the group.\n");
> +			evsel__group_desc(leader, buf, sizeof(buf));
> +			pr_warning("  %s\n", buf);
> +			warned_leader = leader;
> +		}
>  		if (verbose > 0) {
> +			char buf[200];
> +
>  			cpu_map__snprint(leader->core.cpus, buf, sizeof(buf));
>  			pr_warning("     %s: %s\n", leader->name, buf);
>  			cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf));
>  			pr_warning("     %s: %s\n", evsel->name, buf);
>  		}
>  
> -		for_each_group_evsel(pos, leader)
> -			evsel__remove_from_group(pos, leader);
> +		evsel__remove_from_group(evsel, leader);
>  	}
>  }
>  
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo