Date:   Sat, 29 Apr 2023 08:47:45 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrzej Hajda <andrzej.hajda@...el.com>,
        linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [GIT PULL] locking changes for v6.4


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Thu, Apr 27, 2023 at 12:58 PM Ingo Molnar <mingo@...nel.org> wrote:
> >
> >  - Add non-atomic __xchg() variant, use it in a couple of places
> 
> Guys, this is insane, and completely unacceptable.
> 
> I pulled this, but I'm going to unpull it, because the code is
> actively wrong and ugly.
> 
> It not only randomly decides to re-use a name that has existing users
> that now need to be fixed up.

meh - you are 100% right, I'm not sure what we were thinking there ... [ 
actually, I know what we were thinking, but it's a bit complicated - see 
the various non-perfect nomenclature options further below. ]

So the first line of our thinking was that "__" also often & additionally 
means 'lighter weight version of a similar API signature, beware, here be 
dragons, use at your own risk', and more of the focus of these particular 
changes was on identifying hand-coded xchg-ish pieces of code, such as in:

   26ace5d28d36 ("arch/*/uprobes: simplify arch_uretprobe_hijack_return_addr")

... but while that background of '__' is somewhat valid logic that we use 
quite often in various kernel facilities, it doesn't really excuse the 
sloppy decision to slap __ in front of an existing API without trying 
harder, *especially* that a better name with fetch_and_zero() already 
existed :-/

> It then *also* decides to start "preferring" this absolutely
> disgusting new name over a much more legible one in the i915 driver,
> which had this same functionality except it used a prettier name:
> 
>    fetch_and_zero()
> 
> But what then takes the cake for me is that this horribly ugly feature
> then didn't even get that right, and only randomly converted *some* of
> the users, with most of them remaining:
> 
>   git grep fetch_and_zero drivers/gpu/drm/i915/ | wc
>      58     187    5534
>   git grep -w __xchg drivers/gpu/drm/i915/ | wc
>      22     109    1899
> 
> and it looks like the only "logic" to this is that the converted ones
> were in the "gt/" subdirectory. What a random choice, but happily it
> caused a trivial conflict, and as a result I noticed how bad things
> were.
> 
> Anyway, I really find this all offensively ugly and pointless. I'm not
> going to pull some "fixed" version of this. This needs to go away and
> never come back.

Yeah. So I've rebased locking/core to take out these changes - a simple 
revert is too ugly and the history has no value here really.

Will re-send the rest of locking/core.

> What was so magically great about the name "__xchg" that it needed to be 
> taken over by this function? And why was that legibly named version of it 
> replaced so randomly?

Yeah.

So fetch_and_zero() has a bit of a nomenclature & ambiguity problem as 
well: there's already an atomic_fetch_*() API family, and it's easy to 
think that fetch_and_zero() is atomic too - a bit like how xchg() is atomic 
without mentioning 'atomic'.

Adding to the confusion is that there's already atomic APIs that don't use 
atomic_t:

  xchg()
  cmpxchg()
  try_cmpxchg()

... and by *that* implicit nomenclature logic, dropping the atomic_ from a 
atomic_fetch_and_zero() API means: 'atomic API, not using atomic_t'. Which 
fetch_and_zero() clearly isnt ...

So by all that logic and somewhat idiosynchratic API history, the new 
facility should probably not be fetch_and_zero(), but something like 
nonatomic_fetch_and_zero(), but that's quite a mouthful for something so 
simple - and the API family connection to xchg() is lost as well, which is 
a bit sad...

In all that context the least bad approach sounded to add a __ to denote 
__xchg() is 'something special and also lighter weight' (which it is).

I *think* the bigger danger in locking nomenclature is to falsely imply 
atomicity - in that sense I'm not sure fetch_and_zero() is ideal - but I 
can certainly live with it b/c the perfect name keeps eluding me.

> The *whole* point of two underscores is to say "don't use this - it's
> an internal implementation". That's the historical meaning, and it's
> the meaning we have in the kernel too. Two underscores means "this is
> special and doesn't do everything required" (it might need locking
> around it, for example).

Yeah. I do think we might want to keep one related change though:

  e27cff3d2d43 ("arch: rename all internal names __xchg to __arch_xchg")

... not because we want to use the __xchg namespace, but because an _arch 
prefix makes it even *less* likely to be used by non-infrastructure code.

> So then making a new interface with two underscores and thinking "we 
> should now make random drivers use this" is fundamentally bogus.
> 
> Look, just grep for "__xchg" in the main tree (ie the one *without* this 
> change). It all makes sense. It's all clearly an internal helper - as 
> marked by that double underscore - and it's not used by any driver or 
> filesystem code.
> 
> Exactly like K&R and God intended.

Yeah. We'll try this new facility again in v6.5, but with a better name. 
Sorry about that!

Thanks,

	Ingo

Powered by blists - more mailing lists