linux-kernel - Re: [Patch v3 3/5] perf x86/topdown: Don't move topdown metric events in group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <adc34dd8-88bf-4a3e-9939-c2fa0ddb4b65@linux.intel.com>
Date: Tue, 13 Aug 2024 15:30:36 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 Yongwei Ma <yongwei.ma@...el.com>, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v3 3/5] perf x86/topdown: Don't move topdown metric events
 in group


On 8/12/2024 11:37 PM, Liang, Kan wrote:
>
> On 2024-07-12 1:03 p.m., Dapeng Mi wrote:
>> when running below perf command, we say error is reported.
>>
>> perf record -e "{slots,instructions,topdown-retiring}:S" -vv -C0 sleep 1
>>
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x400 (slots)
>>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   disabled                         1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x8000 (topdown-retiring)
>>   { sample_period, sample_freq }   4000
>>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   freq                             1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd 5  flags 0x8
>> sys_perf_event_open failed, error -22
>>
>> Error:
>> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-retiring).
>>
>> The reason of error is that the events are regrouped and
>> topdown-retiring event is moved to closely after the slots event and
>> topdown-retiring event needs to do the sampling, but Intel PMU driver
>> doesn't support to sample topdown metrics events.
>>
>> For topdown metrics events, it just requires to be in a group which has
>> slots event as leader. It doesn't require topdown metrics event must be
>> closely after slots event. Thus it's a overkill to move topdown metrics
>> event closely after slots event in events regrouping and furtherly cause
>> the above issue.
>>
>> Thus don't move topdown metrics events forward if they are already in a
>> group.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/evlist.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
>> index 332e8907f43e..6ae044f21843 100644
>> --- a/tools/perf/arch/x86/util/evlist.c
>> +++ b/tools/perf/arch/x86/util/evlist.c
>> @@ -85,7 +85,12 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>>  		/* Followed by topdown events. */
>>  		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>  			return -1;
>> -		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>> +		/*
>> +		 * Move topdown events forward only when topdown events
>> +		 * are not in same group with previous event.
>> +		 */
> Do you mean this case?
>
> perf stat -e '{slots,branches},topdown-retiring' -C0 sleep 1
> WARNING: events were regrouped to match PMUs
>
>  Performance counter stats for 'CPU(s) 0':
>
>         22,568,316      slots
>            569,904      branches
>          3,805,637      topdown-retiring

Yes, this case can be regrouped.


>
> But if I add one more event before topdown-retiring, it seems break again.
>
> perf stat -e '{slots,branches},cycles,topdown-retiring' -C0 sleep 1
>
>  Performance counter stats for 'CPU(s) 0':
>
>         25,218,108      slots
>            647,598      branches
>          4,345,121      cycles
>    <not supported>      topdown-retiring

Yes, this case can't be supported by original code. I ever tried to support
this format, but it's not easy, it needs to fully change current sort logic.


>
> I'm not asking to support all the above cases. I just try to understand
> which cases you plan to support.
>
> Can you please add some comments or update the document to clearly show
> which format is supported, which format will be automatically adjusted
> by the tool, and which format will be error out?

Yeah, I would list all currently supported regrouping format. BTW, is there
a document to describe the topdown metrics feaeture. If not, I would add
comments here.


>
> We should also need test cases for all the supported formats, not just
> the standard one.

Sure. thanks.


>
> Thanks,
> Kan
>
>> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> +		    lhs->core.leader != rhs->core.leader)
>>  			return 1;
>>  	}
>>