Re: [PATCH v2 09/27] perf parse-events: Create two hybrid hardware events

["Jin, Yao" <yao.jin@linux.intel.com>]

Hi Jiri,

On 3/16/2021 7:05 AM, Jiri Olsa wrote:
> On Thu, Mar 11, 2021 at 03:07:24PM +0800, Jin Yao wrote:
>> For hardware events, they have pre-defined configs. The kernel
>> needs to know where the event comes from (e.g. from cpu_core pmu
>> or from cpu_atom pmu). But the perf type 'PERF_TYPE_HARDWARE'
>> can't carry pmu information.
>>
>> So the kernel introduces a new type 'PERF_TYPE_HARDWARE_PMU'.
>> The new attr.config layout for PERF_TYPE_HARDWARE_PMU is:
>>
>> 0xDD000000AA
>> AA: original hardware event ID
>> DD: PMU type ID
>>
>> PMU type ID is retrieved from sysfs. For example,
>>
>>    cat /sys/devices/cpu_atom/type
>>    10
>>
>>    cat /sys/devices/cpu_core/type
>>    4
>>
>> When enabling a hybrid hardware event without specified pmu, such as,
>> 'perf stat -e cycles -a', two events are created automatically. One
>> is for atom, the other is for core.
> 
> ok I think I understand the need for this (and the following) patch
> the perf_hw_id counters could be global, so when you specify only
> event like:
> 
>     -e cycles
> 
> you want all the cycles, which on hybrid system means cycles from
> more than one pmus
> 

Yes, on hybrid system it means the cycles from two pmus. One cycle is from cpu_core pmu, another 
cycles is from cpu_atom pmu.

> SNIP
> 
>> @@ -1416,6 +1475,8 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
>>   {
>>   	struct perf_event_attr attr;
>>   	LIST_HEAD(config_terms);
>> +	bool hybrid;
>> +	int ret;
>>   
>>   	memset(&attr, 0, sizeof(attr));
>>   	attr.type = type;
>> @@ -1430,6 +1491,18 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
>>   			return -ENOMEM;
>>   	}
>>   
>> +	/*
>> +	 * Skip the software dummy event.
>> +	 */
>> +	if (type != PERF_TYPE_SOFTWARE) {
>> +		if (!perf_pmu__hybrid_exist())
>> +			perf_pmu__scan(NULL);
> 
> this could be checked in the following add_hybrid_numeric call
> 

Yes, that should be OK. I will move the check in the next version.

>> +
>> +		ret = add_hybrid_numeric(parse_state, list, &attr, &hybrid);
>> +		if (hybrid)
>> +			return ret;
>> +	}
> 
> could we add this to separate object.. hybrid.c or maybe parse-events-hybrid.c,
> 
> there's already global __add_event wrapper - parse_events__add_event
> 
> 
> jirka
>

Use a new parse-events-hybrid.c, hmm, well that's OK.

Thanks
Jin Yao

>> +
>>   	return add_event(list, &parse_state->idx, &attr,
>>   			 get_config_name(head_config), &config_terms);
>>   }
>> -- 
>> 2.17.1
>>
>