linux-kernel - Re: [Patch v5 0/6] Bug fixes on topdown events reordering

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

Message-ID: <690ddcd6-276a-4b7b-bd21-fb4ef2349990@linux.intel.com>
Date: Thu, 3 Oct 2024 15:45:11 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Dapeng Mi <dapeng1.mi@...ux.intel.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, Yongwei Ma <yongwei.ma@...el.com>,
 Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering



On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
>>> If the algorithms cannot be changed, can you please give some
>>> suggestions, especially for the sample read failure?
>> So this is symmetric:
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>   return 1;
>> ```
>> That is were lhs and rhs swapped then you'd get the expected comparison order.
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return 1;
>> ```
>> Is symmetric as well.
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return 1;
>> ```
>> (what this patch does) is not symmetric as the group leader impacts
>> the greater-than case but not the less-than case.
>>
>> It is not uncommon to see in a sort function:
>> ```
>> if (cmp(a, b) <= 0) {
>>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
>> ```
> I see.  So are you proposing this?
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 438e4639fa892304..46884fa17fe658a6 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>                 if (arch_is_topdown_slots(rhs))
>                         return 1;
>                 /* Followed by topdown events. */
> -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> +                   lhs->core.leader != rhs->core.leader)
>                         return -1;
>                 /*
>                  * Move topdown events forward only when topdown events
> 
> Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.

I verified the above change. It works well.

Tested-by: Kan Liang <kan.liang@...ux.intel.com>

Thanks,
Kan