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"))
+ return true;
+ }
return false;
}
--
2.40.1
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?
Thanks,
Kan
> + return true;
> + }
>
> return false;
> }
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.
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.
>
> Thanks,
> Kan
>
>> + return true;
>> + }
>>
>> return false;
>> }
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;
>>> }
© 2016 - 2025 Red Hat, Inc.