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

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

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@linux.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;
>>  	}
>>