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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 28 May 2024 11:12:10 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Leo Yan <leo.yan@...ux.dev>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@....com>, Dominique Martinet <asmadeus@...ewreck.org>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] perf evlist: Force adding default events only to core PMUs

On Tue, 28 May 2024 at 10:40, Ian Rogers <irogers@...gle.com> wrote:
>
> I agree it picked the wrong PMU for default events. This was a problem
> on no systems that anybody was bothering to test with. Having been
> made aware of the issue I fixed it in this patch, you're welcome.

You didn't just pick it for default events. You also picked it for
when the user explicitly asks for "profile for cycles"

> What is still not clear from this is what should the behavior be of:
>
> $ perf record -e cycles ...

Why do you claim that?

I've already told you that CLEARLY it's wrong to pick a cycles event
that doesn't support 'record'.

I've also suggested that you might look at core only PMUs.

But more importantly, you should look at documented and historical behavior.

So what is your argument? Because from where I'm sitting, you keep
making irrelevant arguments about *other* events, not about "cycles".

It used to work. It doesn't any more.

> Should it wildcard all events and open them on all PMUs potentially
> failing? Well this has always been perf's behavior were the event:
>
> $ perf record -e inst_retired.any ...

You keep making up irrelevant arguments.

Lookie here: I do "perf list" to just see the events, and what do I
get? Let me quote that for you:

    List of pre-defined events (to be used in -e or -M):
    ...
      cpu-cycles OR cycles                               [Hardware event]

and then later on in the list I get

    general:
      cpu_cycles
           [Cycle. Unit: armv8_pmuv3_0]

and dammit, your patch broke the DOCUMENTED way to get  the most
obvious profiling data: cycles.

So stop making shit up. All your arguments have been bogus garbage
that have been talking about entirely different things than the one
thing I reported was broken.

And you *keep* doing that. Days into this, you keep making shit up
that isn't about this very simple thing.

Every single time I tell you what the problem is, you try to twist to
be about something entirely different. Either a different 'perf'
command entirely, or about a different event that is ENTIRELY
IRRELEVANT.

What the hell is your problem? Why can't you just admit that you
f*cked up, and fix the thing I told you was broken, and that is very
clearly broken and there is no "what about" issues AT ALL.

So stop the idiocy already. Face the actual issue. Don't make up other things.

Dammit, if I wanted "arm_dsu_58/cycles/", I would SAY so. I didn't. I
said "cycles", which is the thing that has always worked across
architectures, that is DOCUMENTED to be the same as "cpu-cycles", and
that used to work just fine.

It's literally RIGHT THERE in "perf list". Using "-e cycles" is
literally also what the man-pages say. This is not me doing something
odd.

And yes, I use an explicit "-e cycles:pp" because the default is not
that. and "cycles:pp" does better than the default.

Again, this is all documented, with "man perf-record" literally
talking about "-e cycles" and then pointing to "man perf-list" for the
modifier details, which detail that 'pp' as meaning "use maximum
detected precise level". Which is exactly what I want (even if on this
machine, it turns out that "p" and "pp" are the same because the armv8
pmuv3  doesn't have that "correct for instruction drift" that Intel
does on x86).

Why is this simple thing so hard for you to understand?

The fact is, if you make "cycles" mean ANYTHING ELSE than the
long-documented actual obvious thing, you have broken perf. It's that
simple.

So stop the excuses already. Stop making up other stuff that isn't
relevant. Stop bringing up events or PMU's that are simply not the
issue.

Face your bug head on, instead of making me have to tell you the same
thing over and over and over again.

                   Linus