Message-ID: <CAP-5=fVGF6A7fwZyF8o0Sz5nNhtXb0007JoCxaiL2XHCKS3=0A@mail.gmail.com>
Date: Tue, 28 May 2024 11:58:57 -0700
From: Ian Rogers <irogers@...gle.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
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, May 28, 2024 at 11:13 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> 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.

But nobody else ever reported the issue, even ARM who maintain the PMU
driver whose event name conflicts. This hasn't been a problem for
anybody else.

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

This is wrong. cycles:P means use the maximum precision, so if the PMU
supports it it will use cycles:ppp and on x86 this is often the case.
The number of 'p's used after the event with :P is read from sysfs:
```
$ cat /sys/devices/cpu/caps/max_precise
3
```
The default using cycles:P is intentional and better than cycles:pp.
If `/sys/devices/cpu/caps/max_precise` is wrong for your PMU driver
then that should get fixed.

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

I keep saying the same thing as I don't agree with you, you have
broken perf in 6.10 and are presumably looking to me to pick up the
pieces. To work around the naming conflict on systems with
arm_dsu_*/events/cycles/ I don't think it is unreasonable to specify
armv8_pmuv3_0/cycles/, the blame for this lies with ARM's event name
within the arm_dsu_* PMU driver, admittedly they didn't know this
would be an issue given perf's non-uniform handling of legacy events.
When ARM requested that legacy events be a lower priority than
sysfs/json then the ball was set in motion for this problem. This was
done to work around an ARM PMU problem on Apple ARM CPUs.

Let's say an Apple CPU has a PMU called armv8_pmuv3_0. If you try to
program a legacy event on it then it will be broken - I lack a system
to test this but I'm reliably informed (user bugs and by ARM) and heck
it was a bunch of work to get this working if it was for nobody. To
fix this perf must read event data for armv8_pmuv3_0/cycles/ from
sysfs. If you don't specify a PMU then perf will try to program a
legacy event. This is the behavior in Linux 6.9. Oh, but legacy events
are broken on my Apple ARM CPU. The change made it so when you don't
specify a PMU you will use the sysfs/json one first instead of the
legacy one. Viola, your favorite `perf record -e cycles:pp ..` should
be fixed on Apple ARM CPUs.

So I think the revert is a real regression for a larger user base.
There is a testing issue here, not least I don't possess an Apple ARM
machine. All of these issues revolve around ARM and yet they do
minimal to try to fix them, beside complain that I should fix the
legacy/sysfs/json issue which is how we got here.

There's of course the alternate universe view that I need to admit I'm
wrong and I should stop going out of my way to break people. Hello
alternate universe, I admit it, I'm wrong and a terrible person please
accept my most sincere apologies. Please alternate universe, can you
tell me how to wild card event names and make them work across PMUs
without the behavior being specific to certain perf comands or event
names (which I think is worse than having to specify which PMU you
think the event should apply)?

Thanks,
Ian

>                    Linus
>

Powered by blists - more mailing lists