[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fU7_RqcG+YO4C=FP_cy__eSd=ieJ_pOe4J-s2zh=sybsw@mail.gmail.com>
Date: Thu, 3 Oct 2024 14:26:29 -0700
From: Ian Rogers <irogers@...gle.com>
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>,
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 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: "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
Powered by blists - more mailing lists