linux-kernel - Re: [PATCH 1/4] perf topdown: Correct leader selection with sample

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] [day] [month] [year] [list]

Message-ID: <8b511e2b-defa-491e-af06-2de85377ba97@linux.intel.com>
Date: Wed, 3 Jul 2024 09:50:42 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: "Mi, Dapeng" <dapeng1.mi@...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,
 Yanfei Xu <yanfei.xu@...el.com>, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [PATCH 1/4] perf topdown: Correct leader selection with
 sample_read enabled



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