Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 7Ec+b49Fbx9cKPjGzPR1fG70etAqPUlvLhdTk1Z4MjrTywKeGg+LUewLOiSa8R5UNut3xxkzB7
 yf3h9rnksyHw==
IronPort-SDR: HdgZrSMvYxMpWD9VdPc6hAXYvZe+YFPZQeLed9OVwtaXUitt7n2DpxpdvLf2lKgtWbxLplY65n
 MG1LpakvHqXQ==
Subject: Re: [PATCH v2 11/27] perf parse-events: Support hardware events
 inside PMU
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com
References: <20210311070742.9318-1-yao.jin@linux.intel.com>
 <20210311070742.9318-12-yao.jin@linux.intel.com> <YEu9usdFl6VSnOQ7@krava>
From:   "Jin, Yao" <yao.jin@linux.intel.com>
Message-ID: <c40d6187-9391-40de-aea8-7389bb369555@linux.intel.com>
Date:   Mon, 15 Mar 2021 10:28:12 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <YEu9usdFl6VSnOQ7@krava>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jiri,

On 3/13/2021 3:15 AM, Jiri Olsa wrote:
> On Thu, Mar 11, 2021 at 03:07:26PM +0800, Jin Yao wrote:
>> On hybrid platform, some hardware events are only available
>> on a specific pmu. For example, 'L1-dcache-load-misses' is only
>> available on 'cpu_core' pmu. And even for the event which can be
>> available on both pmus, the user also may want to just enable
>> one event. So now following syntax is supported:
>>
>> cpu_core/<hardware event>/
>> cpu_core/<hardware cache event>/
>> cpu_core/<pmu event>/
>>
>> cpu_atom/<hardware event>/
>> cpu_atom/<hardware cache event>/
>> cpu_atom/<pmu event>/
>>
>> It limits the event to be enabled only on a specified pmu.
>>
>> The patch uses this idea, for example, if we use "cpu_core/LLC-loads/",
>> in parse_events_add_pmu(), term->config is "LLC-loads".
> 
> hum, I don't understand how this doest not work even now,
> I assume both cpu_core and cpu_atom have sysfs device directory
> with events/ directory right?
> 

Yes, we have cpu_core and cpu_atom directories with events.

root@ssp-pwrt-002:/sys/devices/cpu_atom/events# ls
branch-instructions  bus-cycles    cache-references  instructions  mem-stores  topdown-bad-spec 
topdown-fe-bound
branch-misses        cache-misses  cpu-cycles        mem-loads     ref-cycles  topdown-be-bound 
topdown-retiring

root@ssp-pwrt-002:/sys/devices/cpu_core/events# ls
branch-instructions  cache-misses      instructions   mem-stores  topdown-bad-spec 
topdown-fe-bound   topdown-mem-bound
branch-misses        cache-references  mem-loads      ref-cycles  topdown-be-bound 
topdown-fetch-lat  topdown-retiring
bus-cycles           cpu-cycles        mem-loads-aux  slots       topdown-br-mispredict 
topdown-heavy-ops

> and whatever is defined in events we allow in parsing syntax..
> 
> why can't we treat them like 2 separated pmus?
>

But if without this patch, it reports the error,

root@ssp-pwrt-002:~# ./perf stat -e cpu_core/cycles/ -a -vv -- sleep 1
event syntax error: 'cpu_core/cycles/'
                               \___ unknown term 'cycles' for pmu 'cpu_core'

valid terms: 
event,pc,edge,offcore_rsp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore

Initial error:
event syntax error: 'cpu_core/cycles/'
                               \___ unknown term 'cycles' for pmu 'cpu_core'

valid terms: 
event,pc,edge,offcore_rsp,ldlat,inv,umask,frontend,cmask,config,config1,config2,name,period,percore
Run 'perf list' for a list of valid events

The 'cycles' is treated as a unknown term, then it errors out.

So we have to create another parser to scan the term.

Thanks
Jin Yao

> thanks,
> jirka
> 
>>
>> We create a new "parse_events_state" with the pmu_name and use
>> parse_events__scanner to scan the term->config (the string "LLC-loads"
>> in this example). The parse_events_add_cache() will be called during
>> parsing. The parse_state->pmu_name is used to identify the pmu
>> where the event is enabled.
>>
>> Let's see examples:
>>
>>    root@ssp-pwrt-002:~# ./perf stat -e cpu_core/cycles/,cpu_core/LLC-loads/ -vv -- ./triad_loop
>>    Control descriptor is not initialized
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             6
>>      size                             120
>>      config                           0x400000000
>>      sample_type                      IDENTIFIER
>>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>      disabled                         1
>>      inherit                          1
>>      enable_on_exec                   1
>>      exclude_guest                    1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid 7267  cpu -1  group_fd -1  flags 0x8 = 3
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             7
>>      size                             120
>>      config                           0x400000002
>>      sample_type                      IDENTIFIER
>>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>      disabled                         1
>>      inherit                          1
>>      enable_on_exec                   1
>>      exclude_guest                    1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid 7267  cpu -1  group_fd -1  flags 0x8 = 4
>>    cycles: 0: 449252097 297999924 297999924
>>    LLC-loads: 0: 1857 297999924 297999924
>>    cycles: 449252097 297999924 297999924
>>    LLC-loads: 1857 297999924 297999924
>>
>>     Performance counter stats for './triad_loop':
>>
>>           449,252,097      cpu_core/cycles/
>>                 1,857      cpu_core/LLC-loads/
>>
>>           0.298898415 seconds time elapsed
>>
>>    root@ssp-pwrt-002:~# ./perf stat -e cpu_atom/cycles/,cpu_atom/LLC-loads/ -vv -- taskset -c 16 ./triad_loop
>>    Control descriptor is not initialized
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             6
>>      size                             120
>>      config                           0xa00000000
>>      sample_type                      IDENTIFIER
>>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>      disabled                         1
>>      inherit                          1
>>      enable_on_exec                   1
>>      exclude_guest                    1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid 7339  cpu -1  group_fd -1  flags 0x8 = 3
>>    ------------------------------------------------------------
>>    perf_event_attr:
>>      type                             7
>>      size                             120
>>      config                           0xa00000002
>>      sample_type                      IDENTIFIER
>>      read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>      disabled                         1
>>      inherit                          1
>>      enable_on_exec                   1
>>      exclude_guest                    1
>>    ------------------------------------------------------------
>>    sys_perf_event_open: pid 7339  cpu -1  group_fd -1  flags 0x8 = 4
>>    cycles: 0: 602020010 343657939 342553275
>>    LLC-loads: 0: 3537 343657939 342553275
>>    cycles: 603961400 343657939 342553275
>>    LLC-loads: 3548 343657939 342553275
>>
>>     Performance counter stats for 'taskset -c 16 ./triad_loop':
>>
>>           603,961,400      cpu_atom/cycles/                                              (99.68%)
>>                 3,548      cpu_atom/LLC-loads/                                           (99.68%)
>>
>>           0.344904585 seconds time elapsed
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/util/parse-events.c | 100 +++++++++++++++++++++++++++++++--
>>   tools/perf/util/parse-events.h |   6 +-
>>   tools/perf/util/parse-events.y |  21 ++-----
>>   3 files changed, 105 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 09e42245f71a..30435adc7a7b 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -489,7 +489,8 @@ static int create_hybrid_cache_event(struct list_head *list, int *idx,
>>   static int add_hybrid_cache(struct list_head *list, int *idx,
>>   			    struct perf_event_attr *attr, char *name,
>>   			    struct list_head *config_terms,
>> -			    bool *hybrid)
>> +			    bool *hybrid,
>> +			    struct parse_events_state *parse_state)
>>   {
>>   	struct perf_pmu *pmu;
>>   	int ret;
>> @@ -497,6 +498,11 @@ static int add_hybrid_cache(struct list_head *list, int *idx,
>>   	*hybrid = false;
>>   	perf_pmu__for_each_hybrid_pmu(pmu) {
>>   		*hybrid = true;
>> +		 if (parse_state->pmu_name &&
>> +		     strcmp(parse_state->pmu_name, pmu->name)) {
>> +			continue;
>> +		}
>> +
>>   		ret = create_hybrid_cache_event(list, idx, attr, name,
>>   						config_terms, pmu);
>>   		if (ret)
>> @@ -509,7 +515,8 @@ static int add_hybrid_cache(struct list_head *list, int *idx,
>>   int parse_events_add_cache(struct list_head *list, int *idx,
>>   			   char *type, char *op_result1, char *op_result2,
>>   			   struct parse_events_error *err,
>> -			   struct list_head *head_config)
>> +			   struct list_head *head_config,
>> +			   struct parse_events_state *parse_state)
>>   {
>>   	struct perf_event_attr attr;
>>   	LIST_HEAD(config_terms);
>> @@ -582,7 +589,7 @@ int parse_events_add_cache(struct list_head *list, int *idx,
>>   		perf_pmu__scan(NULL);
>>   
>>   	ret = add_hybrid_cache(list, idx, &attr, config_name ? : name,
>> -			       &config_terms, &hybrid);
>> +			       &config_terms, &hybrid, parse_state);
>>   	if (hybrid)
>>   		return ret;
>>   
>> @@ -1512,6 +1519,11 @@ static int add_hybrid_numeric(struct parse_events_state *parse_state,
>>   	*hybrid = false;
>>   	perf_pmu__for_each_hybrid_pmu(pmu) {
>>   		*hybrid = true;
>> +		if (parse_state->pmu_name &&
>> +		    strcmp(parse_state->pmu_name, pmu->name)) {
>> +			continue;
>> +		}
>> +
>>   		ret = create_hybrid_hw_event(parse_state, list, attr, pmu);
>>   		if (ret)
>>   			return ret;
>> @@ -1578,6 +1590,10 @@ static bool config_term_percore(struct list_head *config_terms)
>>   	return false;
>>   }
>>   
>> +static int parse_events_with_hybrid_pmu(struct parse_events_state *parse_state,
>> +			      const char *str, char *name, bool *found,
>> +			      struct list_head *list);
>> +
>>   int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   			 struct list_head *list, char *name,
>>   			 struct list_head *head_config,
>> @@ -1589,7 +1605,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   	struct perf_pmu *pmu;
>>   	struct evsel *evsel;
>>   	struct parse_events_error *err = parse_state->error;
>> -	bool use_uncore_alias;
>> +	bool use_uncore_alias, found;
>>   	LIST_HEAD(config_terms);
>>   
>>   	if (verbose > 1) {
>> @@ -1605,6 +1621,22 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   		fprintf(stderr, "' that may result in non-fatal errors\n");
>>   	}
>>   
>> +	if (head_config && perf_pmu__is_hybrid(name)) {
>> +		struct parse_events_term *term;
>> +		int ret;
>> +
>> +		list_for_each_entry(term, head_config, list) {
>> +			if (!term->config)
>> +				continue;
>> +			ret = parse_events_with_hybrid_pmu(parse_state,
>> +							   term->config,
>> +							   name, &found,
>> +							   list);
>> +			if (found)
>> +				return ret;
>> +		}
>> +	}
>> +
>>   	pmu = parse_state->fake_pmu ?: perf_pmu__find(name);
>>   	if (!pmu) {
>>   		char *err_str;
>> @@ -1713,12 +1745,19 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>>   	struct perf_pmu *pmu = NULL;
>>   	int ok = 0;
>>   
>> +        if (parse_state->pmu_name) {
>> +                list = alloc_list();
>> +                if (!list)
>> +                        return -1;
>> +                *listp = list;
>> +                return 0;
>> +        }
>> +
>>   	*listp = NULL;
>>   	/* Add it for all PMUs that support the alias */
>> -	list = malloc(sizeof(struct list_head));
>> +	list = alloc_list();
>>   	if (!list)
>>   		return -1;
>> -	INIT_LIST_HEAD(list);
>>   	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
>>   		struct perf_pmu_alias *alias;
>>   
>> @@ -2284,6 +2323,44 @@ int parse_events_terms(struct list_head *terms, const char *str)
>>   	return ret;
>>   }
>>   
>> +static int list_num(struct list_head *list)
>> +{
>> +	struct list_head *pos;
>> +	int n = 0;
>> +
>> +	list_for_each(pos, list)
>> +		n++;
>> +
>> +	return n;
>> +}
>> +
>> +static int parse_events_with_hybrid_pmu(struct parse_events_state *parse_state,
>> +					const char *str, char *pmu_name,
>> +					bool *found, struct list_head *list)
>> +{
>> +	struct parse_events_state ps = {
>> +		.list		= LIST_HEAD_INIT(ps.list),
>> +		.stoken		= PE_START_EVENTS,
>> +		.pmu_name	= pmu_name,
>> +		.idx		= parse_state->idx,
>> +	};
>> +	int ret;
>> +
>> +	*found = false;
>> +	ret = parse_events__scanner(str, &ps);
>> +	perf_pmu__parse_cleanup();
>> +
>> +	if (!ret) {
>> +		if (!list_empty(&ps.list)) {
>> +			*found = true;
>> +			list_splice(&ps.list, list);
>> +			parse_state->idx = list_num(list);
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   int __parse_events(struct evlist *evlist, const char *str,
>>   		   struct parse_events_error *err, struct perf_pmu *fake_pmu)
>>   {
>> @@ -3307,3 +3384,14 @@ char *parse_events_formats_error_string(char *additional_terms)
>>   fail:
>>   	return NULL;
>>   }
>> +
>> +struct list_head *alloc_list(void)
>> +{
>> +	struct list_head *list = malloc(sizeof(*list));
>> +
>> +	if (!list)
>> +		return NULL;
>> +
>> +	INIT_LIST_HEAD(list);
>> +	return list;
>> +}
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index e80c9b74f2f2..39c7121a4659 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -138,6 +138,7 @@ struct parse_events_state {
>>   	struct list_head	  *terms;
>>   	int			   stoken;
>>   	struct perf_pmu		  *fake_pmu;
>> +	char			  *pmu_name;
>>   };
>>   
>>   void parse_events__handle_error(struct parse_events_error *err, int idx,
>> @@ -188,7 +189,8 @@ int parse_events_add_tool(struct parse_events_state *parse_state,
>>   int parse_events_add_cache(struct list_head *list, int *idx,
>>   			   char *type, char *op_result1, char *op_result2,
>>   			   struct parse_events_error *error,
>> -			   struct list_head *head_config);
>> +			   struct list_head *head_config,
>> +			   struct parse_events_state *parse_state);
>>   int parse_events_add_breakpoint(struct list_head *list, int *idx,
>>   				u64 addr, char *type, u64 len);
>>   int parse_events_add_pmu(struct parse_events_state *parse_state,
>> @@ -242,6 +244,8 @@ char *parse_events_formats_error_string(char *additional_terms);
>>   void parse_events_print_error(struct parse_events_error *err,
>>   			      const char *event);
>>   
>> +struct list_head *alloc_list(void);
>> +
>>   #ifdef HAVE_LIBELF_SUPPORT
>>   /*
>>    * If the probe point starts with '%',
>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>> index d57ac86ce7ca..e0e68c3da9e4 100644
>> --- a/tools/perf/util/parse-events.y
>> +++ b/tools/perf/util/parse-events.y
>> @@ -26,18 +26,6 @@ do { \
>>   		YYABORT; \
>>   } while (0)
>>   
>> -static struct list_head* alloc_list(void)
>> -{
>> -	struct list_head *list;
>> -
>> -	list = malloc(sizeof(*list));
>> -	if (!list)
>> -		return NULL;
>> -
>> -	INIT_LIST_HEAD(list);
>> -	return list;
>> -}
>> -
>>   static void free_list_evsel(struct list_head* list_evsel)
>>   {
>>   	struct evsel *evsel, *tmp;
>> @@ -454,7 +442,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT '-' PE_NAME_CACHE_OP_RESULT opt_e
>>   
>>   	list = alloc_list();
>>   	ABORT_ON(!list);
>> -	err = parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6);
>> +	err = parse_events_add_cache(list, &parse_state->idx, $1, $3, $5, error, $6,
>> +				parse_state);
>>   	parse_events_terms__delete($6);
>>   	free($1);
>>   	free($3);
>> @@ -475,7 +464,8 @@ PE_NAME_CACHE_TYPE '-' PE_NAME_CACHE_OP_RESULT opt_event_config
>>   
>>   	list = alloc_list();
>>   	ABORT_ON(!list);
>> -	err = parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4);
>> +	err = parse_events_add_cache(list, &parse_state->idx, $1, $3, NULL, error, $4,
>> +				parse_state);
>>   	parse_events_terms__delete($4);
>>   	free($1);
>>   	free($3);
>> @@ -495,7 +485,8 @@ PE_NAME_CACHE_TYPE opt_event_config
>>   
>>   	list = alloc_list();
>>   	ABORT_ON(!list);
>> -	err = parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2);
>> +	err = parse_events_add_cache(list, &parse_state->idx, $1, NULL, NULL, error, $2,
>> +				parse_state);
>>   	parse_events_terms__delete($2);
>>   	free($1);
>>   	if (err) {
>> -- 
>> 2.17.1
>>
>