Re: Perf stat regression from d15480a3d67

[Andi Kleen <ak@linux.intel.com>]

On Thu, Jun 22, 2023 at 12:09:14PM -0700, Namhyung Kim wrote:
> Hi Andi,
> 
> Sorry about the breakage.
> 
> On Thu, Jun 22, 2023 at 2:20 AM Andi Kleen <ak@linux.intel.com> wrote:
> >
> > > Can you provide an example that is breaking?
> >
> > I only have a very long auto generated example[1], because it's not
> > easy to make a non scheduling event and it's also dependent on the system. The example
> > works on a SKX.
> >
> > >
> > > Wrt event order, cases like:
> > > ```
> > > $ sudo perf stat -e '{data_read,data_write}' -a --no-merge sleep 1
> > >
> > > Performance counter stats for 'system wide':
> > >
> > >          1,960.81 MiB  data_read [uncore_imc_free_running_1]
> > >            438.48 MiB  data_write [uncore_imc_free_running_1]
> > >          1,961.85 MiB  data_read [uncore_imc_free_running_0]
> > >            438.79 MiB  data_write [uncore_imc_free_running_0]
> > >
> > >       1.001127356 seconds time elapsed
> > > ```
> > > Reorder events so that the PMUs match (ie the -e order would 2x
> > > data_read followed by 2x data_write). This covers more cases now so
> > > that perf metric (aka topdown) events aren't broken when coming out of
> > > metrics:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2139
> >
> > How is any tool supposed to handle arbitary output reordering like
> > that? Perf is not just for humans, but also for tools.
> 
> Is the problem of reordering the output or hiding something?
> Do you mean both?

For my tool it's both. In this particular case it's the hiding
that broke it

If the reordering only happens on broken groups etc. as Ian wrote that's ok for me
because I can avoid generating them. But I can't tolerate hiding.

> >
> > >
> > > Given the original commit message there seems little point to give the
> > > 0 counts, but perhaps it is something that could be done on the more
> > > tool oriented CSV and JSON output formats.
> >
> > It's not 0 counts, it's <not supported>. But even 0 counts can
> > be important.
> >
> > We should always output when something doesn't count so that
> > the user knows something is wrong. Hiding problems is always a bad idea
> > and not an improvement.
> 
> Right, I wanted to skip (or hide) events that are not reasonable
> like with redundant or invalid setups.  It should display those
> error results.

Okay so the skipping can just be removed?

> 
> Didn't you say you only have problems in uncore events?

For this test case I only hit it with the uncore, but I would have a general
problem for any event if it was hidden.

Thanks,

-Andi