Re: [PATCH] perf parse: Fix event parser error for hybrid systems

[Arnaldo Carvalho de Melo]

Em Mon, Mar 07, 2022 at 11:16:27PM +0800, zhengjun.xing@xxxxxxxxxxxxxxx escreveu:
> From: Zhengjun Xing <zhengjun.xing@xxxxxxxxxxxxxxx>
> 
> This bug happened on hybrid systems when both cpu_core and cpu_atom
> have the same event name such as "UOPS_RETIRED.MS" while their event
> terms are different, then during perf stat, the event for cpu_atom
> will parse fail and then no output for cpu_atom.

Ian,

	since this patch fixes a patch from you, can you take a look?

- Arnaldo
 
> UOPS_RETIRED.MS -> cpu_core/period=0x1e8483,umask=0x4,event=0xc2,frontend=0x8/
> UOPS_RETIRED.MS -> cpu_atom/period=0x1e8483,umask=0x1,event=0xc2/
> 
> It is because event terms in the "head" of parse_events_multi_pmu_add
> will be changed to event terms for cpu_core after parsing UOPS_RETIRED.MS
> for cpu_core, then when parsing the same event for cpu_atom, it still
> uses the event terms for cpu_core, but event terms for cpu_atom are
> different with cpu_core, the event parses for cpu_atom will fail. This
> patch fixes it, the event terms should be parsed from the original
> event.
> 
> This patch can work for the hybrid systems that have the same event
> in more than 2 PMUs. It also can work in non-hybrid systems.
> 
> Before:
> 
>  #perf stat -v  -e  UOPS_RETIRED.MS  -a sleep 1
> 
> Using CPUID GenuineIntel-6-97-1
> UOPS_RETIRED.MS -> cpu_core/period=0x1e8483,umask=0x4,event=0xc2,frontend=0x8/
> Control descriptor is not initialized
> UOPS_RETIRED.MS: 2737845 16068518485 16068518485
> 
>  Performance counter stats for 'system wide':
> 
>          2,737,845      cpu_core/UOPS_RETIRED.MS/
> 
>        1.002553850 seconds time elapsed
> 
> After:
> 
>  #perf stat -v  -e  UOPS_RETIRED.MS  -a sleep 1
> 
> Using CPUID GenuineIntel-6-97-1
> UOPS_RETIRED.MS -> cpu_core/period=0x1e8483,umask=0x4,event=0xc2,frontend=0x8/
> UOPS_RETIRED.MS -> cpu_atom/period=0x1e8483,umask=0x1,event=0xc2/
> Control descriptor is not initialized
> UOPS_RETIRED.MS: 1977555 16076950711 16076950711
> UOPS_RETIRED.MS: 568684 8038694234 8038694234
> 
>  Performance counter stats for 'system wide':
> 
>          1,977,555      cpu_core/UOPS_RETIRED.MS/
>            568,684      cpu_atom/UOPS_RETIRED.MS/
> 
>        1.004758259 seconds time elapsed
> 
> Fixes: fb0811535e92c6c1 ("perf parse-events: Allow config on kernel PMU events")
> Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Signed-off-by: Zhengjun Xing <zhengjun.xing@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/util/parse-events.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 9739b05b999e..8bf7f914ea0e 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1648,6 +1648,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  {
>  	struct parse_events_term *term;
>  	struct list_head *list = NULL;
> +	struct list_head *orig_head = NULL;
>  	struct perf_pmu *pmu = NULL;
>  	int ok = 0;
>  	char *config;
> @@ -1674,7 +1675,6 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  	}
>  	list_add_tail(&term->list, head);
>  
> -
>  	/* Add it for all PMUs that support the alias */
>  	list = malloc(sizeof(struct list_head));
>  	if (!list)
> @@ -1687,13 +1687,15 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  
>  		list_for_each_entry(alias, &pmu->aliases, list) {
>  			if (!strcasecmp(alias->name, str)) {
> +				parse_events_copy_term_list(head, &orig_head);
>  				if (!parse_events_add_pmu(parse_state, list,
> -							  pmu->name, head,
> +							  pmu->name, orig_head,
>  							  true, true)) {
>  					pr_debug("%s -> %s/%s/\n", str,
>  						 pmu->name, alias->str);
>  					ok++;
>  				}
> +				parse_events_terms__delete(orig_head);
>  			}
>  		}
>  	}
> -- 
> 2.25.1

-- 

- Arnaldo