Re: [PATCH 1/4] perf topdown: Correct leader selection with sample_read enabled

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2024-07-02 10:46 p.m., Mi, Dapeng wrote:
> 
> On 7/3/2024 12:05 AM, Liang, Kan wrote:
>>
>> On 2024-07-02 6:40 p.m., Dapeng Mi wrote:
>>> Addresses an issue where, in the absence of a topdown metrics event
>>> within a sampling group, the slots event was incorrectly bypassed as
>>> the sampling leader when sample_read was enabled.
>>>
>>> perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1
>>>
>>> In this case, the slots event should be sampled as leader but the
>>> branches event is sampled in fact like the verbose output shows.
>>>
>>> perf_event_attr:
>>>   type                             4 (cpu)
>>>   size                             168
>>>   config                           0x400 (slots)
>>>   sample_type                      IP|TID|TIME|READ|CPU|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                             0 (PERF_TYPE_HARDWARE)
>>>   size                             168
>>>   config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
>>>   { sample_period, sample_freq }   10000
>>>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>>>   read_format                      ID|GROUP|LOST
>>>   sample_id_all                    1
>>>   exclude_guest                    1
>>>
>>> The sample period of slots event instead of branches event is reset to
>>> 0.
>>>
>>> This fix ensures the slots event remains the leader under these
>>> conditions.
>>>
>>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>>> ---
>>>  tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>>> index 3f9a267d4501..5d7b78eb7516 100644
>>> --- a/tools/perf/arch/x86/util/topdown.c
>>> +++ b/tools/perf/arch/x86/util/topdown.c
>>> @@ -1,6 +1,7 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  #include "api/fs/fs.h"
>>>  #include "util/evsel.h"
>>> +#include "util/evlist.h"
>>>  #include "util/pmu.h"
>>>  #include "util/pmus.h"
>>>  #include "util/topdown.h"
>>> @@ -41,11 +42,22 @@ bool topdown_sys_has_perf_metrics(void)
>>>   */
>>>  bool arch_topdown_sample_read(struct evsel *leader)
>>>  {
>>> +	struct evsel *event;
>>> +
>>>  	if (!evsel__sys_has_perf_metrics(leader))
>>>  		return false;
>>>  
>>> -	if (leader->core.attr.config == TOPDOWN_SLOTS)
>>> -		return true;
>>> +	if (leader->core.attr.config != TOPDOWN_SLOTS)
>>> +		return false;
>>> +
>>> +	/*
>>> +	 * If slots event as leader event but no topdown metric events in group,
>>> +	 * slots event should still sample as leader.
>>> +	 */
>>> +	evlist__for_each_entry(leader->evlist, event) {
>>> +		if (event != leader && strcasestr(event->name, "topdown"))
>> User may uses the RAW format. It may not be good enough to just check
>> the event name.
>>
>> I recall you have a complete support for this in the previous patch. Why
>> drop it?
> 
> 
> Oh, I ignored the RAW format case. Yes, there is a complete comparison in
> previous patch, but I originally thought it's over-complicated, so I just
> simplified it (refer other helpers to compare the name).  If we need to
> consider the RAW format, it may be not correct for the comparisons in the
> helpers arch_evsel__must_be_in_group() and arch_evlist__cmp() as well.
>

Right, those need to be fixed as well.

> If we want to fix the issue thoroughly, we may have to expose two helpers
> which check if an event is topdown slots or metrics event and use these two
> helpers to replace current name comparison.

Yes, you may have to add an extra patch to introduce the two helper
functions and replace the existing function.

Thanks,
Kan
> 
>>
>> Thanks,
>> Kan
>>
>>> +			return true;
>>> +	}
>>>  
>>>  	return false;
>>>  }