Re: [PATCH v1] perf evlist: Force adding default events only to core PMUs

[Linus Torvalds]

On Mon, 27 May 2024 at 22:37, Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> If you do:
>
> $ perf stat -e cycles ...

You always talk about "perf stat", because you want to ignore a big
part of the issueL

> The issue is about legacy events.

No.

The issue isn't "perf stat".

That works. Even with the broken version.

> I have a patch that's WIP for this, but I also think we could
> also agree that when >1 PMU advertises an event, perf's behavior when
> matching should be to open all such events. You avoid this by
> specifying a PMU name.

Christ. You're still ignoring the elephant in the room.

Stop using "perf stat" as an example.  It's bogus. You're ignoring the issue.

Lookie here:

    $ perf stat make
    ...
     Performance counter stats for 'make':

              5,262.78 msec task-clock                       #
1.195 CPUs utilized
                46,891      context-switches                 #
8.910 K/sec
                     6      cpu-migrations                   #
1.140 /sec
               198,402      page-faults                      #
37.699 K/sec
        10,238,763,227      cycles                           #
1.946 GHz
        16,280,644,955      instructions                     #    1.59
 insn per cycle
         3,252,743,314      branches                         #
618.066 M/sec
            83,702,386      branch-misses                    #
2.57% of all branches

           4.405792739 seconds time elapsed

           4.287784000 seconds user
           0.921132000 seconds sys

but then

    $ perf record make
    Error:
    cycles:P: PMU Hardware doesn't support
sampling/overflow-interrupts. Try 'perf stat'

because that broken thing (a) picked a different cycles than before
and (b) your argument that it should pick both IS WRONG BECAUSE ONE OF
THEM DOESN'T EVEN WORK.

Why is this so hard to just accept? Why do you keep mis-stating the problem?

How hard is it to realize that I DO NOT WANT "perf stat"? The perf
error message is bogus crap. If I ask for a "perf record", it
shouldn't pick the wrong PMU that can't do it, and then say "do you
want to do something else"?

I don't care a whit for "legacy events". I care about the fact that
you changed the code to pick the WRONG event, and then you are blaming
anything but that.

If perf would go "Oh, this one doesn't support 'record', let's see if
another one does", it would all be good.

If perf would go "Oh, let's prioritize core events", it would all be good.

But no. Your patch actively picked a bad event, and then you try to
blame some "legacy" thing.

Yes, the legacy thing picked the right event, but it's not even about
legacy. You could have picked the right event any number of other
ways.

It's about "it damn well worked when you didn't go out of your way to
pick the wrong event".

In other words, this isn't about "legacy" and "new".

This is about "right" and "wrong". The old code picked right - for
whatever reasons. The new code picked wrong - also for whatever
reasons.

Don't try to make it be anything else. Just admit that the new code
picked the wrong PMU, instead of trying to make excuses for it.

               Linus