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

Liang, Kan
2024-07-03T13:50:45Z

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;
>>> }