Message-ID: <CAP-5=fV1wvWqQs3oEojdv4YDB-CFOFSotJoLAZdn_m1+AxRZQw@mail.gmail.com>
Date: Mon, 7 Oct 2024 22:13:22 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Mi, Dapeng1" <dapeng1.mi@...el.com>
Cc: "Liang, Kan" <kan.liang@...ux.intel.com>, 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

On Mon, Oct 7, 2024 at 7:52 PM Mi, Dapeng1 <dapeng1.mi@...el.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ian Rogers <irogers@...gle.com>
> > Sent: Friday, October 4, 2024 7:36 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 4:29 PM Liang, Kan <kan.liang@...ux.intel.com> wrote:
> > >
> > >
> > >
> > > On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
> > > >> 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-nex
> > > >> t.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.
> > > > I got this:
> > > >
> > > >   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}"
> > true
> > > >   Error:
> > > >   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for
> > event (topdown-be-bound).
> > > >   "dmesg | grep -i perf" may provide additional information.
> > >
> > > To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
> > > is a meaningless case. Why a user wants to group instructions and
> > > topdown events, but leave the slots out of the group?
> > > There could be hundreds of different combinations caused by the perf
> > > metrics mess. I don't think the re-ordering code should/can fix all of them.
> >
> > I'm happy with better code and things don't need to be perfect. Can we fix the
> > comments though? It'd be nice to also include that some things are going to be
> > broken. I can imagine groups with topdown events but without slots, for example
> > we group events in metrics and in tma_retiring we add "0 *
> > tma_info_thread_slots" to the metric so that we get a slots event. If the multiply
> > were optimized away as redundant then we'd have a topdown group without
> > slots, we could pick up slots and other events from other metrics.
>
> I don't think this patch would break any current regroup case. It just blocks to move topdown metrics event if they are already in same group with slot events. We can see same error for this event combination "slots,cycles,{instructions,topdown-be-bound}" in the original code.
>
> As Kan said, there could be hundreds of topdown metrics combinations, it's unrealistic to cover all these combinations just using sorting, and even it can be done, the comparator would become much complicated and hard to maintain.
>
> I think we'd better just maintain several commonly used regroup cases, it would be fine to raise an error for these unsupported combinations as long as error message is clear enough.
>
> Ian, I don't fully understand your words, could you please give an example? Thanks.

So in the comparator I think most people won't understand the list of
cases that are supported and those that are not supported:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf-tools-next#n35
The groups usually come from metrics and to work around issues there
are constraints on those that may or may not group events. This could
yield situations that don't work given the cases you list, but I don't
think current metrics will violate this. We do test metrics
individually but not together as part of perf test.

Thanks,
Ian

> >
> > > For the case which the re-ordering cannot cover (like above), an error
> > > out is acceptable. So the end user can update their command to a more
> > > meaningful format, either {slots,cycles,instructions,topdown-be-bound}
> > > or {slots,topdown-be-bound},cycles,instructions still works.
> >
> > Perhaps we can add an arch error path that could help more for topdown events
> > given they are a particular pain to open.
> >
> > > I think what the patch set really fixed is the failure of sample read
> > > with perf metrics. Without the patch set, it never works no matter how
> > > you change the order of the events.
> > > A better ordering is just a nice to have feature. If perf cannot
> > > provides a perfect re-ordering, I think an error out is also OK.
> >
> > Agreed, we don't need to fix everything and focussing on the common use cases
> > makes sense.
> >
> > Thanks,
> > Ian

Powered by blists - more mailing lists