LWN: Comments on "A disagreement over get_mm_exe_file()"

A disagreement over get_mm_exe_file()

polyp — Thu, 28 Oct 2021 09:30:38 +0000

I think the developer, which seems experienced, should know better than to push for a patch this stupid. I mean, come on, changing the behavior of a driver based on the name of the executable? In the mainline kernel. That's just beyond crazy....

Shocked at Hellwig’s behavior

Wol — Wed, 27 Oct 2021 18:45:08 +0000

All it needs is for Linus to weigh in with a "what were you thinking about" flame.

But it probably does need Linus to weigh in and say "cut this stuff out!" A few Linus fireballs and that would almost certainly do the trick ...

Cheers,
Wol

Shocked at Hellwig’s behavior

nilsmeyer — Wed, 27 Oct 2021 17:37:02 +0000

I think the public nature of it all can cause it to spiral quickly since both are "performing" in frot of an audience.

A disagreement over get_mm_exe_file()

blackwood — Wed, 20 Oct 2021 21:27:04 +0000

Just runs afoul of the kernels "never a regression when only upgrading the kernel" rule. Which apparently we ended up tossing in here because the solution is too awful to merge or something like that.

In userspace (well mesa3d) we do this all properly with quirk options and separate config file that matches on binaries and a few other things. And we ship a default config file with known necessary quirks.

A disagreement over get_mm_exe_file()

mjg59 — Wed, 20 Oct 2021 19:15:03 +0000

How about a more general solution - provide a DRM-level sysfs interface that allows process name/quirk pairs to be added at runtime and then just make it a userland problem?

A disagreement over get_mm_exe_file()

mfuzzey — Wed, 20 Oct 2021 18:24:48 +0000

Is the new (cros carries the patch) solution that bad?

It should only be temporary until cros can fix their user space code.

Also cros is a full OS with their own Linux tree so it's already set up to handle kernel patches.

Agreed that out of tree patches aren't good but when they're temporary like this I think that's better than polluting the upstream kernel indefinitely.

A disagreement over get_mm_exe_file()

blackwood — Wed, 20 Oct 2021 15:36:08 +0000

It was accepted because we discussed it on irc. It is a terrible option, but also it's a least terrible option if you want upstream to both Just Work everywhere as much as possible, and also not block drivers from not being able to enable features.

The solution is now that we force cros to carry more downstream patches, and there was just a talk at lpc about how that's not a great thing for anyone involved.

Occasionally real world engineering just sucks.

A disagreement over get_mm_exe_file()

blackwood — Wed, 20 Oct 2021 15:33:03 +0000

It would neither affect your chrome or chromium browser nor your electron apps, because they are not compositors, and do not run on top of the kernel modesetting driver directly.

They do use the render side of drm, but that on is not quirked. Only when you have a chrome process that's actually doing kms compositing will any hack kick in, and in that case you do want it. So if you look at it it's by far not as silly as it looks at first glance, and there's not actually any known bad impact where we do not want it.

A disagreement over get_mm_exe_file()

immibis — Wed, 20 Oct 2021 10:31:40 +0000

As you can see, they are stored in some kind of database, not just hard-coded willy-nilly.

A disagreement over get_mm_exe_file()

excors — Wed, 20 Oct 2021 10:00:02 +0000

I suspect Windows has many different ways of managing compatibility hacks, but one of those ways is basically comparing the executable name. See HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers in the registry - mine has a few entries for applications I've installed, like "C:\Program Files (x86)\Steam\steamapps\common\Quake\Glquake.exe"=DWM8And16BitMitigation ("The fix offers mitigation for applications that work in 8/16-bit display color mode because these legacy color modes are not supported in Windows 8" according to https://docs.microsoft.com/en-us/windows/deployment/plann...).

There's also some that appear to be pre-set by Windows (since they're for applications I've never used), e.g. HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\PenService lists a bunch of executables like "iexplore.exe" and "photoshop.exe", and then sets the DisableFlickGestureDetection flag on (I think) application-specific window class names.

A disagreement over get_mm_exe_file()

immibis — Wed, 20 Oct 2021 08:48:55 +0000

I suspect Windows has a more sophisticated way of managing compatibility hacks than !strcmp(process->exe_file->name, "chrome")

A disagreement over get_mm_exe_file()

taladar — Wed, 20 Oct 2021 08:34:11 +0000

It would likely also affect all those electron and similar apps embedding chrome. Matching the binary name just seems like a really, really bad solution for Chrome bugs in particular, even worse than in general.

Shocked at Hellwig’s behavior

pbonzini — Wed, 20 Oct 2021 06:16:24 +0000

The kind of depressing part is that it shouldn't be too hard. Like in the first message of the thread you just s/utter crap/bad ideas/ and you've done 99% of the job.

That said, asking "Have you heard about the kernel no-regression rule?" sarcastically is not respectful either, and like most sarcasm it just comes across as silly/petty to me. So it's not like either side has an especially high ground.

CoC enforcement

lkundrak — Wed, 20 Oct 2021 05:25:22 +0000

Exactly my thoughts. If anything, it seems to me that the person in question needs help.

A disagreement over get_mm_exe_file()

pizza — Wed, 20 Oct 2021 01:46:58 +0000

It was a workaround in Windows 3.1 for the DOS version of Sim City.

https://www.joelonsoftware.com/2004/06/13/how-microsoft-l...

A disagreement over get_mm_exe_file()

ilammy — Wed, 20 Oct 2021 01:25:52 +0000

This reminds me of the story how old Windows had a hack in its kernel to avoid Age of Empires game crash, also specifically disabling some paging thing for AoE binaries.

(Side note: I find it very sad that I was not able to find the source of this story because all search engine outputs are polluted all sorts of irrelevant things.)

Shocked at Hellwig’s behavior

josefbacik — Wed, 20 Oct 2021 00:59:04 +0000

> Even if this is the widespread response — does t the community want more people to align on what is “right” over time? If so, should teaching, writing policy documentation that can be referenced, and otherwise acting like effective engineering leaders be preferred?

Yes exactly, that is the ideal outcome. My point is there's no pressure mechanism to push the community in that direction, the majority of senior developers and maintainers are comfortable with the status quo. I want it to change, but I'm tired and simply don't want to it anymore, and I suspect I'm not alone in that, so the end result will be more of the same for the foreseeable future.

Shocked at Hellwig’s behavior

ms-tg — Tue, 19 Oct 2021 23:26:21 +0000

> There really isn't anything to be done. The CoC doesn't mean anything if there aren't consequences to actions. The common refrain is "this wasn't OK, but X was right." Because in the end being right is the thing the community cares about the most, not about being professional.

Even if this is the widespread response — does t the community want more people to align on what is “right” over time? If so, should teaching, writing policy documentation that can be referenced, and otherwise acting like effective engineering leaders be preferred?

A disagreement over get_mm_exe_file()

atnot — Tue, 19 Oct 2021 20:58:45 +0000

I'm sure it'll be a relief for prospective kernel developers to learn that they will not have abuse hurled at them for submitting well intentioned patches.

...that is of course, unless someone somewhere suddenly arbitrarily decides that it's acceptable and also they deserve it.

Shocked at Hellwig’s behavior

josefbacik — Tue, 19 Oct 2021 20:26:57 +0000

There really isn't anything to be done. The CoC doesn't mean anything if there aren't consequences to actions. The common refrain is "this wasn't OK, but X was right." Because in the end being right is the thing the community cares about the most, not about being professional.

And what are you going to do? Tell one of the most prolific developers to go away for X time and think about what they've done? This is a common attitude for a lot of the most senior maintainers in the community, there doesn't appear to be any will to have anything real come from violating the CoC. And even if there was punishment, what's to say that changes behavior? Changing the behavior and culture is far more valuable than simply temporarily banning people and hoping they change their ways on their own, but a much harder thing to accomplish.

To answer your last question, they wouldn't. Even people like me who have been around for a long time are asking themselves the same question, and for myself have decided that I'm going to step away after I wrap up a few outstanding projects. The cost of dealing with the more hard to deal with members of this community is becoming too high of a price to pay. There's always new blood that will come in and take up space, but lots that just won't engage in the first place, and a few that will decide they're done and go do something else.

Shocked at Hellwig’s behavior

ms-tg — Tue, 19 Oct 2021 17:40:53 +0000

If I didn’t know this was real, I might have thought this was comedy exaggerated for effect, to show how horrible Linux maintainers behave.

What can be done? I was under the impression that Linus was committed to protecting the community from this sort of behavior where, instead of teaching or documenting a policy decision to be referenced in the future, or laying out a case to build alignment around avoiding unintended consequences, the maintainer repeatedly attacked the person.

The contrast to workplaces, where this would never be tolerated, is shocking to me in 2021.

Why would anyone willingly subject themselves to an engineering culture where this is tolerated?

A disagreement over get_mm_exe_file()

ddevault — Tue, 19 Oct 2021 16:01:17 +0000

Yeah, that is a fair thing to be concerned about.

CoC enforcement

pbonzini — Tue, 19 Oct 2021 15:56:25 +0000

It's not a great example, to be honest. It paints the CoC more as a tool to avoid annoyances, than as a way to improve the community overall.

A disagreement over get_mm_exe_file()

pbonzini — Tue, 19 Oct 2021 15:52:15 +0000

Sure, I'm all for improved language and attitude, though in this case improvements could have been made from both sides. From a purely technical point of view, I still wonder how the patch could have been accepted by the maintainers, and if it would have been caught before it reached linux.git.

A disagreement over get_mm_exe_file()

ddevault — Tue, 19 Oct 2021 13:59:35 +0000

Like I said upfront, Simon's patch was not great. That's no excuse for abusive language on the mailing list.

A disagreement over get_mm_exe_file()

pbonzini — Tue, 19 Oct 2021 13:56:14 +0000

> was deemed sufficient for inclusion in drm-next

And I'm surprised about that, because Christoph makes good points: "Independent of that a check for a specific program as the callers makes no sense whatsoever as a given program and change over time. (here) You cement in a mapping of an executable name to map to what you consider a "bug" without any way to fix it up. Which is even worse for a something fast moving like chrome/chromeos which will eventually gets its act together and fix things while you'll keep a weird feature mismatch just for it around forever. (here)".

We can discuss the form of his complaint, but I honestly do agree with him that this should not have made it past the maintainers.

CoC enforcement

corbet — Tue, 19 Oct 2021 13:53:20 +0000

It's rare, but it does occasionally happen; here's an example.

A disagreement over get_mm_exe_file()

ttuttle — Tue, 19 Oct 2021 13:36:18 +0000

Chrome adding that patch to their local kernel is absolutely an option! I just think it doesn't belong upstream at all.

A disagreement over get_mm_exe_file()

hwentland — Tue, 19 Oct 2021 13:25:38 +0000

Slide 8 if anyone is interested in the fd.o CoC transparency report: https://indico.freedesktop.org/event/1/contributions/24/a...

A disagreement over get_mm_exe_file()

hwentland — Tue, 19 Oct 2021 13:20:54 +0000

> if Chrome OS tries to enable the cursor plane when the overlay plane is being used incompatibly, refuse to do it

Doing so crashes the ChromeOS compositor, which isn't nice to do and something we'd like to avoid. Hence, Simon's patch.

I wasn't entirely happy with Simon's patch but it fixed an awkward issue that was specific to AMD HW which is why I decided to take it. Carrying the patch in the Chrome tree until its compositor is fixed seems to be the best solution.

A disagreement over get_mm_exe_file()

blackwood — Tue, 19 Oct 2021 10:29:57 +0000

fd.o does, but there's some obvious limits on what that achieves of course.

A disagreement over get_mm_exe_file()

pbonzini — Tue, 19 Oct 2021 06:25:14 +0000

I am not sure I will ever see the Linux code of conduct enforced...

A disagreement over get_mm_exe_file()

airlied — Mon, 18 Oct 2021 20:32:30 +0000

The problem being with I_AM_NOT_BROKEN ioctls is that how do you fix something after the horse has bolted.

Revert early works fine if you catch misbehaviour early, if you don't then you have to fall back to more extreme measures.

We have one in the drm where X.org used an API in a really bad way a year or two after the API has been deployed. We ended up putting a new API trapdoor for future X.org but for current X.org matching the name was the least offensive answer. Invariable people start adding heuristics to "detect" behaviour patterns which often end up worse.

That and hch was being an asshole for no reason. He knows he was, and we do have a CoC.

A disagreement over get_mm_exe_file()

flussence — Mon, 18 Oct 2021 19:46:30 +0000

It sounds like there's a whole comedy of errors here...

1. Google sucks. Fix your own trillion-dollar software!

2. I'm speaking out of line here, but a kernel-side GPU driver sniffing a browser user-agent to hide bugs is one of the most insane things I've ever seen. What's more is there already are hundreds of I_AM{_NOT,}_BROKEN ioctl special cases for things like this, but they belong in /usr/share/drirc.d/, not /usr/src/linux/.

3. The fact that it's still possible to wedge the GPU from userspace at all, let alone through exposed APIs. If the old API is that dangerous to expose in parallel then just make it a wrapper or emulation layer for the new one, and make the legacy users swallow any performance hit. It's not like anyone will care about the weight of a few thousand extra lines of workarounds in *the amdgpu driver*.

A disagreement over get_mm_exe_file()

ddevault — Mon, 18 Oct 2021 18:01:56 +0000

Flames are simply inappropriate, unconditionally. The conduct policy is not contingent on the quality of the patch.

A disagreement over get_mm_exe_file()

Wol — Mon, 18 Oct 2021 17:54:21 +0000

Hmm...

If it's a well-reviewed patch, then yes flames *are* inappropriate ...

Cheers,
Wol

A disagreement over get_mm_exe_file()

ddevault — Mon, 18 Oct 2021 17:19:44 +0000

I don't think it really matters, but Ser is an experienced hacker outside of the kernel domain, an expert in the Linux graphics stack, and has contributed 80 patches to Linux to date. But like all of his patches, this one was reviewed by several other people and discussed at length and was deemed sufficient for inclusion in drm-next, only to run into cross-tree problems which no one could have foreseen. It was only when it inconvenienced Christoph that he felt that it was appropriate to levy abuse at Simon.

A disagreement over get_mm_exe_file()

Wol — Mon, 18 Oct 2021 17:15:30 +0000

Is Ser a new hacker, or an old hand who should know better?

Linus has learnt that flames are not wise in any circumstances, but most of his were aimed at lieutenants who should have known better.

Christoph, well I think I've had the odd run-in with him, and he might not yet have learnt that lesson ...

Cheers,
Wol

A disagreement over get_mm_exe_file()

ddevault — Mon, 18 Oct 2021 16:54:26 +0000

I wasn't in love with Ser's patch, but the response from Hellwig was totally uncalled for.

Good opportunity to remind everyone that there's a code of conduct for Linux now:

https://www.kernel.org/code-of-conduct.html

The old days of intelligent jerks is not acceptable in modern kernel development. Kernel hackers don't spring forth from the earth ready to write perfect patches - novices become experts only with good mentoring, not with berating and rude comments.

A disagreement over get_mm_exe_file()

ttuttle — Mon, 18 Oct 2021 15:37:36 +0000

This is rather trivially broken, as "chromium" on Chromium OS may exhibit the same problem and won't be recognized as quirky.

I'd expect the driver to implement something of a finer-grained interlock -- if Chrome OS tries to enable the cursor plane when the overlay plane is being used incompatibly, refuse to do it! If an app is using the two APIs at once, it gets to keep both pieces when they interact poorly.

The resulting quirk should then be implemented in Chrome OS, not in the kernel -- maybe that means checking the driver name, testing to see if the cursor plane can be enabled on top of a non-fullscreen overlay plane, and avoiding use of the overlay plane if so.