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: <PH0PR11MB48246886EE0A79DE5EA613FECD7E2@PH0PR11MB4824.namprd11.prod.outlook.com>
Date: Tue, 8 Oct 2024 02:30:38 +0000
From: "Mi, Dapeng1" <dapeng1.mi@...el.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>, 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>, "Hunter, Adrian"
	<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-perf-users@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Yongwei Ma
	<yongwei.ma@...el.com>
Subject: RE: [Patch v5 0/6] Bug fixes on topdown events reordering



> -----Original Message-----
> From: Liang, Kan <kan.liang@...ux.intel.com>
> Sent: Friday, October 4, 2024 3:45 AM
> 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>; Hunter, Adrian
> <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>; Mi, Dapeng1
> <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>

Since the linux.intel.com mail server is down, I have to use the intel.com mail server (outlook client) to respond this mail. The mail format may vary. Sorry for this.

Thanks Kan for testing this.

Ian, thanks for pointing that the comparators are not symmetrical. I agree it would lead to an inconsistent sorting results if changing comparing condition from "cmp(a, b) < 0" to "cmp(b, a) > 0" or vice versa, but limit to some certain sort library like perf-tool, the sorting result should be still fixed, right?

Anyway, I would provide a new patch to make the comparators are symmetrical. Thanks.

> 
> Thanks,
> Kan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ