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: <PH0PR11MB48247D4A9F1D09156A9DA765CD7E2@PH0PR11MB4824.namprd11.prod.outlook.com>
Date: Tue, 8 Oct 2024 02:31:32 +0000
From: "Mi, Dapeng1" <dapeng1.mi@...el.com>
To: Ian Rogers <irogers@...gle.com>, "Liang, Kan" <kan.liang@...ux.intel.com>
CC: Namhyung Kim <namhyung@...nel.org>, 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: Ian Rogers <irogers@...gle.com>
> Sent: Friday, October 4, 2024 5:26 AM
> To: Liang, Kan <kan.liang@...ux.intel.com>
> Cc: Namhyung Kim <namhyung@...nel.org>; 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 Thu, Oct 3, 2024 at 12:45 PM Liang, Kan <kan.liang@...ux.intel.com> wrote:
> >
> >
> >
> > 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>
> 
> Dapeng's comment should cover replace the comment /* Followed by topdown
> events. */ but there are other things amiss. I'm thinking of something like:

Thanks. I would change the comments.

> "slots,cycles,{instructions,topdown-be-bound}" the topdown-be-bound should get
> sorted and grouped with slots, but cycles and instructions have no reason to be
> reordered, so do we end up with slots, instructions and topdown-be-bound being
> grouped with cycles sitting ungrouped in the middle of the evlist? I believe there
> are assumptions that grouped evsels are adjacent in the evlist, not least
> in:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-
> next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> Does cycles instructions end up being broken out of a group in this case? Which
> feels like the case the code was trying to avoid.
> 
> Thanks,
> Ian