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;
Date:   Tue, 16 Mar 2021 00:05:05 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Jin Yao <yao.jin@linux.intel.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
Subject: Re: [PATCH v2 09/27] perf parse-events: Create two hybrid hardware
 events
Message-ID: <YE/oIQq9XHIsKVIj@krava>
References: <20210311070742.9318-1-yao.jin@linux.intel.com>
 <20210311070742.9318-10-yao.jin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210311070742.9318-10-yao.jin@linux.intel.com>
Precedence: bulk

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

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

> +
> +		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

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