Re: [drm/i915/gt] 8c3b1ba0e7: perf-sanity-tests.Parse_event_definition_strings.fail

[Jin, Yao]

Hi Jiri,

On 2/26/2021 3:03 AM, Jiri Olsa wrote:
> On Thu, Feb 25, 2021 at 07:50:46PM +0100, Jiri Olsa wrote:
> > On Thu, Feb 25, 2021 at 07:16:44PM +0100, Jiri Olsa wrote:
> > > On Thu, Feb 25, 2021 at 02:33:57PM +0100, Jiri Olsa wrote:
> > > > On Thu, Feb 25, 2021 at 04:06:23PM +0800, Jin, Yao wrote:
> > > > > Hi Chris, Arnaldo, Jiri,
> > > > >
> > > > > We observe the parsing error for "software/xxx/" on some platforms.
> > > > >
> > > > > For example,
> > > > >
> > > > > # perf stat -e software/r1a/ -a -- sleep 1
> > > > > event syntax error: 'software/r1a/'
> > > > >                       \___ parser error
> > > > > Run 'perf list' for a list of valid events
> > > > >
> > > > >   Usage: perf stat [<options>] [<command>]
> > > > >
> > > > >      -e, --event <event>   event selector. use 'perf list' to list available events
> > > > >
> > > > > And perf test is failed too.
> > > > >
> > > > > # perf test 6 -vv
> > > > > ...
> > > > > running test 4 'software/r1a/'failed to parse event 'software/r1a/', err 1, str 'parser error'
> > > > > event syntax error: 'software/r1a/'
> > > > >                       \___ parser error
> > > > >
> > > > > running test 4 'software/r0x1a/'failed to parse event 'software/r0x1a/', err 1, str 'parser error'
> > > > > event syntax error: 'software/r0x1a/'
> > > > >                       \___ parser error
> > > > > ...
> > > > >
> > > > > The issue should be the conflict between event "i915/software-gt-awake-time/" and pmu "software".
> > > > >
> > > > > # perf list | grep i915/software
> > > > >    i915/software-gt-awake-time/                       [Kernel PMU event]
> > > > >
> > > > > Perf supports the event format as "prefix-xxx-suffix", so the "software"
> > > > > string in "software-gt-awake-time" is added to perf_pmu_events_list as
> > > > > PMU_EVENT_SYMBOL_PREFIX. When parsing the string such as "software/xxx/",
> > > > > it’s parsed as PMU_EVENT_SYMBOL_PREFIX for "software", then it would error
> > > > > out in next processing.
> > > > >
> > > > > So the easy way is to rename "software-gt-awake-time" to "gt-awake-time",
> > > > > right? Otherwise we have to hardcode something in perf tool.
> > > > >
> > > > > What do you think of this issue and the solution?
> > > > >
> > > > > BTW, as the robot reported, it's related to the commit 8c3b1ba0e7ea
> > > > > ("drm/i915/gt: Track the overall awake/busy time").
> > > >
> > > > yes, I think you're right.. I'll try to reproduce and check on this
> > > >
> > > > I'm still not clear why is that parsing fail if those 2 events are
> > > > distinguished by - and / ... but yep, we are asking for trouble in
> > > > here ;-)
> > > >
> > > > I think we could either prioritaze one or add extra check when we
> > > > add prefix-xxx-suffix events and do not add ones that have conflict..
> > > > that seems like best solution to me now
> > >
> > > would patch below work for you?
> > >
> > > jirka
> >
> > and now with more fixes after I ran tests ;-)
> >
> > we should perhaps add also same check for non '-' aliases, I'll check
>
> and one more version ;-)
>
> jirka
>
>
> ---
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 42c84adeb2fb..d9e795056d15 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2028,30 +2028,49 @@ static void perf_pmu__parse_init(void)
>   		perf_pmu_events_list_num = -1;
>   		return;
>   	}
> -	perf_pmu_events_list = malloc(sizeof(struct perf_pmu_event_symbol) * len);
> +	perf_pmu_events_list = zalloc(sizeof(struct perf_pmu_event_symbol) * len);
>   	if (!perf_pmu_events_list)
>   		return;
> -	perf_pmu_events_list_num = len;
>   
>   	len = 0;
>   	pmu = NULL;
>   	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
>   		list_for_each_entry(alias, &pmu->aliases, list) {
>   			struct perf_pmu_event_symbol *p = perf_pmu_events_list + len;
> -			char *tmp = strchr(alias->name, '-');
> +			char *prefix = strdup(alias->name);
> +			char *tmp = strchr(prefix, '-');
> +
> +			if (tmp != NULL)
> +				*tmp = 0;
> +
> +			/*
> +			 * Skip any prefix that conflicts with pmu name,
> +			 * otherwise the event parser gets confused.
> +			 */
> +			if (perf_pmu__find(prefix)) {
> +				pr_debug2("perf_pmu__parse_init: skipping conflicting alias '%s'\n",
> +					  alias->name);
> +				free(prefix);
> +				continue;
> +			}
>   
>   			if (tmp != NULL) {
> -				SET_SYMBOL(strndup(alias->name, tmp - alias->name),
> -						PMU_EVENT_SYMBOL_PREFIX);
> +				SET_SYMBOL(prefix, PMU_EVENT_SYMBOL_PREFIX);
>   				p++;
>   				SET_SYMBOL(strdup(++tmp), PMU_EVENT_SYMBOL_SUFFIX);
>   				len += 2;
>   			} else {
> -				SET_SYMBOL(strdup(alias->name), PMU_EVENT_SYMBOL);
> +				SET_SYMBOL(prefix, PMU_EVENT_SYMBOL);
>   				len++;
>   			}
>   		}
>   	}
> +
> +	/* unlikely, but still.. */
> +	if (!len)
> +		goto err;
> +	perf_pmu_events_list_num = len;
> +
>   	qsort(perf_pmu_events_list, len,
>   		sizeof(struct perf_pmu_event_symbol), comp_pmu);
>   

Thanks so much for the patch! It works with my tests.

# ./perf test 6
 6: Parse event definition strings                                  : Ok

# ./perf stat -e software/r1a/ -a -- sleep 1

 Performance counter stats for 'system wide':

   <not supported>      software/r1a/

       1.000940433 seconds time elapsed

In theory, do we also need to check suffix as well? I think returning PMU_EVENT_SYMBOL_SUFFIX may 
also confuse the parser. But yes, we don't have this case now.

Thanks
Jin Yao