Message-ID: <2025020506-vineyard-replay-1801@gregkh>
Date: Wed, 5 Feb 2025 16:36:37 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Vince Weaver <vincent.weaver@...ne.edu>
Cc: Ian Rogers <irogers@...gle.com>, Stephane Eranian <eranian@...gle.com>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	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>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH] perf/core: move all of the pmu devices into their own
 location

On Wed, Feb 05, 2025 at 10:06:55AM -0500, Vince Weaver wrote:
> On Wed, 5 Feb 2025, Greg Kroah-Hartman wrote:
> 
> > On Tue, Feb 04, 2025 at 08:21:17PM -0500, Vince Weaver wrote:
> > > 
> > > it doesn't matter if it's a one line change, your proposed change breaks 
> > > userspace.
> > 
> > It breaks broken userspace :)
> 
> no, it breaks working code that's been working for 15 years, apparently on 
> some whim by you.

These devices have been here for 15 years?  I know I've been ignoring
them for a while, but I don't think that long.  Let me check, ok at
least 15, but there was not many event sources back then, and we didn't
really notice it.

Then in 2023, there was an attempt to fix up parent events in commit
143f83e2003a ("perf: Allow a PMU to have a parent"), which does properly
put pmu devices within the device tree.

So if for no other reason, the userspace tools need to be fixed up to
find those devices, they are broken as of 2023 as they can't find them
in the tree unless they follow the in-kernel documentation that says to
look in /sys/bus/event_source/*/devices (as documented in
Documentation/admin-guide/perf/* and many many Documentation/ABI/
entries.

Also, modern systems have way more "perf objects" than before, so
dumping them all at the root of sysfs is not ok, it might not have been
noticed in the past, but on modern systems today, it is.

> I thought we didn't break userspace.

We try not to, yes.  Again, this is an area where we do change all the
time, and in fact, sysfs was designed to allow this.  That's the rule of
sysfs, never treat the /sys/devices/ tree as stable.  Seems that some
tools didn't get that memo, so I'll go fix them up, not a big deal.

> > > libpfm4/PAPI are widely used and deployed and in some cases possibly 
> > > statically linked into other projects.  It will take years for the change 
> > > to filter out to all users.
> > 
> > Great, as it is easy to get this type of fix into the stable kernels,
> > and then fix these tools, by the time those old systems eventually
> > update to a newer kernel version, it should "just work".
> 
> no, that is the worst case scenario.  PAPI is installed on older systems, 
> often custom non-vendor packages.  So if you get this into stable then 
> these older RHEL systems the kernel will be updated suddenly breaking the 
> users with no warning.

So you really update RHEL systems with a custom kernel?  That kind of
defeats the purpose of RHEL, and wastes your support money you are
paying for it :)

RHEL can handle this just fine, they will update their packages
properly over time, I can wait.

> Even if we manage to get a change into libpfm4 and PAPI it will probably 
> be months before the next stable release, and again people tend not to 
> upgrade PAPI releases that often.

Fair enough, what is the release cycle of those tools?

> > I'm fixing up the bug where all of these devices accidentally got dumped
> > into the root of the device tree in the system.
> 
> is this a "bug" or just you shuffling around files for no reason?

It is a "bug" in that the devices should never have been there in the
first place, AND that userspace should never assume that /sys/devices is
stable.

It's also a "bug" in that these tools might end up trying to touch
devices they really aren't supposed to be touching as assuming anything
with an "events" subdir in the root of sysfs is going to break anyway on
their own.

Heck, I'm tempted to create a "/sys/devices/reset_me/events/" object now
just to see what kind of tools trip over that :)

> > Again, /sys/devices/ is NEVER guaranteed to have specific placement,
> > that's the whole point of sysfs in the first place.  We can, and do,
> > move devices around in there all the time (even every other boot), it's
> > just that some tools accidentally didn't realize this and now need to be
> > fixed.
> 
> great.  Slap a note about this happening in the appropriate "deprecated" 
> file with a timeline of 5 years out or whatever, and then when that comes 
> around then send your patch.

I'll send an updated patch soon and put it behind a config option for
those systems that want to do the right thing, update userspace, and
then remove the config option in a few years.  We've done it in the
past, not a big deal.

thanks for the review.

greg k-h

Powered by blists - more mailing lists