From: "Jin, Yao" <yao.jin@linux.intel.com>
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
Subject: Re: [PATCH] perf evsel: Adjust hybrid event and global event mixed group
Date: Tue, 8 Jun 2021 21:24:49 +0800 [thread overview]
Message-ID: <1110d89c-b948-7e72-08cb-dbbf2cca2d65@linux.intel.com> (raw)
In-Reply-To: <YL9M+FUE5BHaD9w/@krava>
Hi Jiri,
On 6/8/2021 6:56 PM, Jiri Olsa wrote:
> On Tue, Jun 01, 2021 at 09:38:27AM +0800, Jin Yao wrote:
>> A group mixed with hybrid event and global event is allowed. For example,
>> group leader is 'intel_pt//' and the group member is 'cpu_atom/cycles/'.
>>
>> e.g.
>> perf record --aux-sample -e '{intel_pt//,cpu_atom/cycles/}:u'
>>
>> The challenge is their available cpus are not fully matched. For example,
>> 'intel_pt//' is available on CPU0-CPU23, but 'cpu_atom/cycles/' is
>> available on CPU16-CPU23.
>>
>> When getting the group id for group member, we must be very careful.
>> Because the cpu for 'intel_pt//' is not equal to the cpu for
>> 'cpu_atom/cycles/'. Actually the cpu here is the index of evsel->core.cpus,
>> not the real CPU ID.
>>
>> e.g. cpu0 for 'intel_pt//' is CPU0, but cpu0 for 'cpu_atom/cycles/' is CPU16.
>>
>> Before:
>>
>> # perf record --aux-sample -e '{intel_pt//,cpu_atom/cycles/}:u' -vv uname
>> ...
>> ------------------------------------------------------------
>> perf_event_attr:
>> type 10
>> size 128
>> config 0xe601
>> { sample_period, sample_freq } 1
>> sample_type IP|TID|TIME|CPU|IDENTIFIER
>> read_format ID
>> disabled 1
>> inherit 1
>> exclude_kernel 1
>> exclude_hv 1
>> enable_on_exec 1
>> sample_id_all 1
>> exclude_guest 1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 4084 cpu 0 group_fd -1 flags 0x8 = 5
>> sys_perf_event_open: pid 4084 cpu 1 group_fd -1 flags 0x8 = 6
>> sys_perf_event_open: pid 4084 cpu 2 group_fd -1 flags 0x8 = 7
>> sys_perf_event_open: pid 4084 cpu 3 group_fd -1 flags 0x8 = 9
>> sys_perf_event_open: pid 4084 cpu 4 group_fd -1 flags 0x8 = 10
>> sys_perf_event_open: pid 4084 cpu 5 group_fd -1 flags 0x8 = 11
>> sys_perf_event_open: pid 4084 cpu 6 group_fd -1 flags 0x8 = 12
>> sys_perf_event_open: pid 4084 cpu 7 group_fd -1 flags 0x8 = 13
>> sys_perf_event_open: pid 4084 cpu 8 group_fd -1 flags 0x8 = 14
>> sys_perf_event_open: pid 4084 cpu 9 group_fd -1 flags 0x8 = 15
>> sys_perf_event_open: pid 4084 cpu 10 group_fd -1 flags 0x8 = 16
>> sys_perf_event_open: pid 4084 cpu 11 group_fd -1 flags 0x8 = 17
>> sys_perf_event_open: pid 4084 cpu 12 group_fd -1 flags 0x8 = 18
>> sys_perf_event_open: pid 4084 cpu 13 group_fd -1 flags 0x8 = 19
>> sys_perf_event_open: pid 4084 cpu 14 group_fd -1 flags 0x8 = 20
>> sys_perf_event_open: pid 4084 cpu 15 group_fd -1 flags 0x8 = 21
>> sys_perf_event_open: pid 4084 cpu 16 group_fd -1 flags 0x8 = 22
>> sys_perf_event_open: pid 4084 cpu 17 group_fd -1 flags 0x8 = 23
>> sys_perf_event_open: pid 4084 cpu 18 group_fd -1 flags 0x8 = 24
>> sys_perf_event_open: pid 4084 cpu 19 group_fd -1 flags 0x8 = 25
>> sys_perf_event_open: pid 4084 cpu 20 group_fd -1 flags 0x8 = 26
>> sys_perf_event_open: pid 4084 cpu 21 group_fd -1 flags 0x8 = 27
>> sys_perf_event_open: pid 4084 cpu 22 group_fd -1 flags 0x8 = 28
>> sys_perf_event_open: pid 4084 cpu 23 group_fd -1 flags 0x8 = 29
>> ------------------------------------------------------------
>> perf_event_attr:
>> size 128
>> config 0x800000000
>> { sample_period, sample_freq } 4000
>> sample_type IP|TID|TIME|PERIOD|IDENTIFIER|AUX
>> read_format ID
>> inherit 1
>> exclude_kernel 1
>> exclude_hv 1
>> freq 1
>> sample_id_all 1
>> exclude_guest 1
>> aux_sample_size 4096
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 4084 cpu 16 group_fd 5 flags 0x8
>> sys_perf_event_open failed, error -22
>>
>> The group_fd 5 is not correct. It should be 22 (the fd of
>> 'intel_pt' on CPU16).
>>
>> After:
>>
>> # perf record --aux-sample -e '{intel_pt//,cpu_atom/cycles/}:u' -vv uname
>> ...
>> ------------------------------------------------------------
>> perf_event_attr:
>> type 10
>> size 128
>> config 0xe601
>> { sample_period, sample_freq } 1
>> sample_type IP|TID|TIME|CPU|IDENTIFIER
>> read_format ID
>> disabled 1
>> inherit 1
>> exclude_kernel 1
>> exclude_hv 1
>> enable_on_exec 1
>> sample_id_all 1
>> exclude_guest 1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 5162 cpu 0 group_fd -1 flags 0x8 = 5
>> sys_perf_event_open: pid 5162 cpu 1 group_fd -1 flags 0x8 = 6
>> sys_perf_event_open: pid 5162 cpu 2 group_fd -1 flags 0x8 = 7
>> sys_perf_event_open: pid 5162 cpu 3 group_fd -1 flags 0x8 = 9
>> sys_perf_event_open: pid 5162 cpu 4 group_fd -1 flags 0x8 = 10
>> sys_perf_event_open: pid 5162 cpu 5 group_fd -1 flags 0x8 = 11
>> sys_perf_event_open: pid 5162 cpu 6 group_fd -1 flags 0x8 = 12
>> sys_perf_event_open: pid 5162 cpu 7 group_fd -1 flags 0x8 = 13
>> sys_perf_event_open: pid 5162 cpu 8 group_fd -1 flags 0x8 = 14
>> sys_perf_event_open: pid 5162 cpu 9 group_fd -1 flags 0x8 = 15
>> sys_perf_event_open: pid 5162 cpu 10 group_fd -1 flags 0x8 = 16
>> sys_perf_event_open: pid 5162 cpu 11 group_fd -1 flags 0x8 = 17
>> sys_perf_event_open: pid 5162 cpu 12 group_fd -1 flags 0x8 = 18
>> sys_perf_event_open: pid 5162 cpu 13 group_fd -1 flags 0x8 = 19
>> sys_perf_event_open: pid 5162 cpu 14 group_fd -1 flags 0x8 = 20
>> sys_perf_event_open: pid 5162 cpu 15 group_fd -1 flags 0x8 = 21
>> sys_perf_event_open: pid 5162 cpu 16 group_fd -1 flags 0x8 = 22
>> sys_perf_event_open: pid 5162 cpu 17 group_fd -1 flags 0x8 = 23
>> sys_perf_event_open: pid 5162 cpu 18 group_fd -1 flags 0x8 = 24
>> sys_perf_event_open: pid 5162 cpu 19 group_fd -1 flags 0x8 = 25
>> sys_perf_event_open: pid 5162 cpu 20 group_fd -1 flags 0x8 = 26
>> sys_perf_event_open: pid 5162 cpu 21 group_fd -1 flags 0x8 = 27
>> sys_perf_event_open: pid 5162 cpu 22 group_fd -1 flags 0x8 = 28
>> sys_perf_event_open: pid 5162 cpu 23 group_fd -1 flags 0x8 = 29
>> ------------------------------------------------------------
>> perf_event_attr:
>> size 128
>> config 0x800000000
>> { sample_period, sample_freq } 4000
>> sample_type IP|TID|TIME|PERIOD|IDENTIFIER|AUX
>> read_format ID
>> inherit 1
>> exclude_kernel 1
>> exclude_hv 1
>> freq 1
>> sample_id_all 1
>> exclude_guest 1
>> aux_sample_size 4096
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 5162 cpu 16 group_fd 22 flags 0x8 = 30
>> sys_perf_event_open: pid 5162 cpu 17 group_fd 23 flags 0x8 = 31
>> sys_perf_event_open: pid 5162 cpu 18 group_fd 24 flags 0x8 = 32
>> sys_perf_event_open: pid 5162 cpu 19 group_fd 25 flags 0x8 = 33
>> sys_perf_event_open: pid 5162 cpu 20 group_fd 26 flags 0x8 = 34
>> sys_perf_event_open: pid 5162 cpu 21 group_fd 27 flags 0x8 = 35
>> sys_perf_event_open: pid 5162 cpu 22 group_fd 28 flags 0x8 = 36
>> sys_perf_event_open: pid 5162 cpu 23 group_fd 29 flags 0x8 = 37
>> ------------------------------------------------------------
>> ...
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> ---
>> tools/perf/util/evsel.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index 4a3cd1b5bb33..a9cf615fe580 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1581,6 +1581,15 @@ int __evsel__read_on_cpu(struct evsel *evsel, int cpu, int thread, bool scale)
>> return 0;
>> }
>>
>> +static int evsel_cpuid_match(struct evsel *evsel1, struct evsel *evsel2,
>> + int cpu)
>> +{
>
> could this be better name:
>
> evsel__match_other_cpu(struct evsel *evsel, struct evsel *other, int cpu)
>
>
OK. I will use evsel__match_other_cpu in v2.
>> + int cpuid;
>> +
>> + cpuid = perf_cpu_map__cpu(evsel1->core.cpus, cpu);
>> + return perf_cpu_map__idx(evsel2->core.cpus, cpuid);
>> +}
>> +
>> static int get_group_fd(struct evsel *evsel, int cpu, int thread)
>> {
>> struct evsel *leader = evsel->leader;
>> @@ -1595,6 +1604,26 @@ static int get_group_fd(struct evsel *evsel, int cpu, int thread)
>> */
>> BUG_ON(!leader->core.fd);
>>
>> + /*
>> + * If leader is global event (e.g. 'intel_pt//'), but member is
>> + * hybrid event. Need to get the leader's fd from correct cpu.
>> + */
>> + if (evsel__is_hybrid(evsel) &&
>> + !evsel__is_hybrid(leader)) {
>> + cpu = evsel_cpuid_match(evsel, leader, cpu);
>> + BUG_ON(cpu == -1);
>> + }
>> +
>> + /*
>> + * Leader is hybrid event but member is global event.
>> + */
>> + if (!evsel__is_hybrid(evsel) &&
>> + evsel__is_hybrid(leader)) {
>> + cpu = evsel_cpuid_match(evsel, leader, cpu);
>> + if (cpu == -1)
>> + return -1;
>> + }
>
> why do we call BUG_ON on the first one and return -1 on the other?
> they are equally bad no?
>
For first one, evsel1 is hybrid event and evsel2 is global event. The evsel1 is only available on
part of CPUs and evsel2 is avaialbe on total CPUs.
So 'perf_cpu_map__idx(evsel2->core.cpus, cpuid);' must return a valid cpu index otherwise there
should be a bug here.
For second one, evsel1 is global event and evsel2 is hybrid event. The evsel2 is only available on
part of CPUs. It's possible that cpuid is not in the range of evsel2->core.cpus. So it's not a bug.
> could you put that into separate function, like
>
> cpu = evsel__hybrid_group_fd(evsel, cpu);
>
Sure, that's OK.
Thanks
Jin Yao
> jirka
>
>
>> +
>> fd = FD(leader, cpu, thread);
>> BUG_ON(fd == -1);
>>
>> --
>> 2.17.1
>>
>
prev parent reply other threads:[~2021-06-08 13:25 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-01 1:38 Jin Yao
2021-06-08 10:56 ` Jiri Olsa
2021-06-08 13:24 ` Jin, Yao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1110d89c-b948-7e72-08cb-dbbf2cca2d65@linux.intel.com \
--to=yao.jin@linux.intel.com \
--cc=Linux-kernel@vger.kernel.org \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=kan.liang@intel.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=yao.jin@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®