Re: Event reordering regression for software events

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Sun, Jul 16, 2023 at 07:00:44PM -0700, Andi Kleen escreveu:
> On Thu, Jul 13, 2023 at 08:56:55PM -0700, Ian Rogers wrote:
> > Yes I disagree. We can't always force perf metric events in a group
> > because weak groups don't always work. When we place the perf metric
> > events outside of a group they are broken. So now we don't group the
> > events and fix the topdown events to be in a group, in arch specific
> > code. It is a complete pain brought about by the Intel core PMU
> > driver.
> 
> Was that the old topdown slots have to be first issue, or something
> else?
> 
> If it's so painful I guess could revisit the kernel part, although
> I suspect anything that the kernel could do the perf tool could do too.
> 
> > 
> > > So why does the perf tool need to fix it? Just pass it to the kernel
> > > and it won't schedule and the user has to fix it.
> > 
> > Because in some cases, and the cases I have happened to care about,
> > the groups of events come from metrics.
> 
> That's fine, but can't you just fix it for the metrics only.
> 
> Also even for metrics if you freely reorder them they might become
> unparseable.  But the events inside the metrics should be freely

Don't we have a JSON output? My expectation is that after the work that
Ian has been doing we will settle down, but even then downstreamers
should try to use JSON as input as that would avoid the problems we are
having?

You mentioned that it was difficult to add entries to 'perf test' but
that we could try running toplev's regression test, right? I can add it
to my set of tests but perhaps the best thing would be to wire it up to
one of the CIs out there?

- Arnaldo

> reorderable (well technically someone could have a dependency on that
> too, but at least in my mind that's much less likely than their
> dependency just being on the metric) 
> 
> So what it really needs is a sort only insight the metric.
> 
> That shouldn't be too hard to implement, right?
> 
> Also I guess we could add some test cases on avoiding reordering,
> although I'm optimistic that once it is established that reordering
> should be minimized/avoided it is unlikely to be something 
> that breask frequently.
> 
> Would it be possible to revert the global sort patch until that 
> is sorted out? Otherwise I'm afraid I'll end up with perf releases
> that are absolutely incompatible to toplev, and I'll get problems
> with my user base who often cannot easily update perf.
> 
> Arnaldo, can you please comment on this, what is your policy 
> on regressions here?

 
> > But things like wildcard group expansion and regrouping is a metric
> > problem too. So why would there be a parse events for metrics and a
> > separate one for events? Doing it this way would make metric group
> > testing significantly more hard too, as we can't just try the events
> > without the metric.
> 
> Hmm, perhaps add some kind of debug flag to handle this case?
> As long as it's not default I don't have any issues with that.
> 
> > > How about replacing the sorting commit with a warning and ask
> > > the user to do the reordering? Would that work for you?
> > 
> > No.
> 
> Well I hope we can get to some compromise here. Right now 
> this arbitary unpredictable reordering is a show stopper for me,
> and I suspect it will be for any other perf stat users that do some kind
> of post processing.
> 
> As long as we find a solution like above that works for the metrics
> it should be ok right?
> 
> > > The platform is SKX.
> > >
> > > >
> > > > > > would seem unfortunate. Tbh, the mistake here is the assumptions that
> > > > > > toplev is making.
> > > > >
> > > > > It's not an random assumption, it's the only way to parse perf output uniquely.
> > > > >
> > > > > Please make it an option. But this cannot stay this way unconditionally
> > > > > if perf stat is supposed to stay a tool which can be used from other programs.
> > > >
> > > > You should be using the json or CSV outputs. These are intended for
> > >
> > > This is the CSV output that is reordered! (see the example you just
> > > quoted)
> > >
> > > I haven't tested JSON, but I assume that is reordered too.
> > 
> > The output of the JSON is a dictionary and inherently unordered.
> 
> I haven't tried to use it, but if you have something like
> perf stat --json -e '{cycles,branches},{cycles,cache-misses}' how would you
> distinguish the two different cycles without order? 
> 
> > crashes vs having the complete TopdownL1 working on both PMUs, testing
> > parity with non-hybrid. We also fixed all address and leak sanitizer
> > bugs. 
> 
> > Perf in Linux 6.5 even without the awesome new features like
> > lock contention analysis, is the best perf tool we've ever had.
> 
> Sure and without arbitrary reordering and unparseable output it would be
> even better ...
> 
> > Does SKX have uncore? Yes, so you need to reorder events and this
> > change didn't start it.
> 
> Yes of course it has an uncore.
> 
> You only need to reorder if you create broken groups.  At least my tool
> doesn't generate them.
> 
> > NO_GROUP_EVENTS is used to avoid a group being used, an important use
> > being avoiding PMU driver bugs.
> > 
> 
> Hmm, not sure what that is refering to, but ok.
> 
> -Andi

-- 

- Arnaldo