RE: [Patch v5 0/6] Bug fixes on topdown events reordering

[Mi, Dapeng1]

> -----Original Message-----
> From: Liang, Kan <kan.liang@xxxxxxxxxxxxxxx>
> Sent: Friday, October 4, 2024 3:45 AM
> To: Namhyung Kim <namhyung@xxxxxxxxxx>; Ian Rogers <irogers@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>;
> Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>; Hunter, Adrian
> <adrian.hunter@xxxxxxxxx>; Alexander Shishkin
> <alexander.shishkin@xxxxxxxxxxxxxxx>; Dapeng Mi
> <dapeng1.mi@xxxxxxxxxxxxxxx>; linux-perf-users@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Yongwei Ma <yongwei.ma@xxxxxxxxx>; Mi, Dapeng1
> <dapeng1.mi@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>

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