Message-ID: <20240417184040.GEZiAXqKvw3uRcmTJL@fat_crate.local>
Date: Wed, 17 Apr 2024 20:40:40 +0200
From: Borislav Petkov <bp@...en8.de>
To: Uros Bizjak <ubizjak@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Waiman Long <longman@...hat.com>, x86@...nel.org
Subject: Re: [tip: locking/core] locking/atomic/x86: Introduce
 arch_try_cmpxchg64_local()

On Wed, Apr 17, 2024 at 06:24:21PM +0200, Uros Bizjak wrote:
> We are dealing with locking primitives, probably the hottest part of
> the kernel. For 64-bits, the patch is effectively a couple of lines,
> reusing and extending existing macros,

Ok.

> the line count for a modern 32-bit target is also a couple of lines,
> but there the saved insn count is much higher, around 10 instructions.

Yah, that __arch_try_cmpxchg64_emu_local() thing with yet another
alternative in there. So that's not a couple of lines - it is yet
another cryptic alternative we need to pay attention to.

> Really? Was this decision reached by the community consensus?

Nothing official. Unofficially, we don't care.

> The linux kernel has many uses, and using it for servers by a big
> company, you are the voice of,

No, here I'm wearing my maintainer hat.

> I'm sure that 32-bit is quite relevant for embedded users and more

People keep dangling those "embedded users" each time. Which users are
those? I haven't seen anyone complaining about 32-bit kernels being
broken or testing them. Because we keep breaking them and no one
notices. Maybe that's a sign for how much they're used.

Although, I broke 32-bit recently and people caught it so there are some
straddlers from time to time. But that's very seldom. And each time we
tell them to switch to 64-bit.

> than relevant to a student or an enthusiast in some remote part of the
> world.

Trust me, they have 64-bit CPUs. Most of the 32-bit CPUs they had are
probably dead already. Like mine.

32-bit only CPUs like P4, for example, should be trashed just because
they're contributing to global warming. :-P

> As a maintainer, you should also take care of the communities
> that are somehow neglected, where unilateral decisions like the one
> above can have unwanted consequences.

We still keep 32-bit kernels alive - no one has dropped them yet - we
just don't add new features.

> If the line count is the problem, I can easily parametrize new and
> existing big macro descriptions in a follow-up patch. However, I was
> advised to not mix everything together in one patch, but rest assured,
> the creation and testing of the follow-up patch would take me less
> time than writing the message you are reading.

I'm simply making sure we're not going off the rails with
micro-optimizing for no apparent reason.

Saving a

	test   %rax,%rax

doesn't need fixing in my book. Because I don't think you'll be able to
even measure it.

> It brings no future maintenance burden, but it perhaps improves
> someone's life a tiny bit.

This is where you and I disagree: touching that alternative in
__arch_try_cmpxchg64_emu_local() does as we tend to change them from
time to time, especially in recent times.

And I wouldn't mind touching it but if it is there to save 10 insns on
32-bit - which doesn't matter - then why bother?

Or do you have a relevant 32-bit workload which brings any improvement
by this change?

> Last, but not least, I'm bringing some ideas from the compiler
> development community, where the attitude to redundant instructions is
> totally different. It could take weeks of effort and considerable
> rewrite of compiler functionality just to remove one instruction ;)
> Micro-optimizations add up!

I'm sure but they all need to be weighed in. Zapping a TEST REG,REG is
not worth it. On most machines, that ALU insn executes in 1 cycle.

I wanna say, such "optimizations" should be checked by benchmarks to see
whether they even give any improvements but we can't check every patch.

IOW, all the patches we're adding should answer the "Is it really worth
the effort?" question. And don't forget that "it brings no future
maintenance burden" is wrong. It brings a maintenance burden every time
we refactor the kernel. And we do that all the time. So the more sites
you have to touch, the more it adds up.

So even if your patch saves 10 insns but there's not a single workload
where it matters, then don't bother. There are other, lower hanging
fruits we need to tackle first.

> Thanks for reading this,

Thanks for taking the time to explain how you're seeing it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists