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

[Namhyung Kim]

On Thu, Oct 03, 2024 at 04:36:25PM -0700, Ian Rogers wrote:
> On Thu, Oct 3, 2024 at 4:29 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> 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-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.
> > > 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

Can you please propose something?

Thanks,
Namhyung


> 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.