RE: [PATCH] perf/x86/amd/power: Use div64_u64 onstead of do_div()

[David Laight <David.Laight@ACULAB.COM>]

From: Linus Torvalds
> Sent: 05 May 2024 20:39
> 
> On Sat, 4 May 2024 at 15:53, David Laight <David.Laight@aculab.com> wrote:
> >
> > I was thinking of something like (see https://godbolt.org/z/h8oj7h4sd):
> >  [...]
> > Which will call out for a large divisor, but will do inlined divides
> > otherwise - so is reasonably sane whatever the types
> 
> No. This is not sane.
> 
> Once you have a callout, you might as well do it all entirely out of
> line. You've messed up your register state and instruction scheduling
> opportunities, and the advantage of inlining part of the computation
> is basically almost entirely gone.
> 
> And the thing is, if you do a 64-by-64 divide, then you just do it.
> Gcc will create that disgusting out-of-line code for you.
> 
> BUT IT SUCKS.
> 
> We don 't do stupid things in the kernel. We basically never need the
> 64-by-64 divide.

There are 381 references to div64_u64() some of them are just plain stupid.
Constant divisors of 100 and 1000 and div64_u64(x_ns * 100, y_us * 1000).
Have a look :-)

> The reason we have "do_div()" and friends is EXATCLY
> THAT - to catch the "oh, you did something stuipid, just fix it".
> 
> Yes, we do divide 64-bit entities even on 32-bit hosts, but 99% of the
> time the range of the divisor is basically always limited, and gcc
> sadly does not understand that that case matters.

The problem is that code does do_div(u64, unsigned long) (which is fine).
But someone wrote a script that complains that is a full 64bit divide
on 64bit systems - so suggests div64_u64().
They someone thinks they can fix an easy 'bug' by doing the substitution.
Except the code was fine (due to the domain of the divisor) and the
modified code is entirely broken due the different calling conventions.
(Never mind that it is likely slow on 32bit.)

div64_u64() really needs a 'must check' attribute.

So the idea was to inline a 64x32 divide and do a call for a 64x64 one.
But include an optimisation that avoided the call for a 64x64 divide
if (at run time) the divisor was only 32bit.
(How much does a 'ret' cost these days with all the mitigations?)

Adding !__builtin_constant_p(_bhi) to the check for the call removes
the run-time check - but will still inline u64/u64 if the compiler
knows the divisor is small.

Given people won't use the right function, optimising div64_u64()
for divisors that are known at compile time to be 32bits has
to make some sense.

I'm trying to think how to do a compile time NULL test for the
'pointer to remainder' for an inline function.
Maybe if (!__builtin_const_p(r) || (r)) *(r) = _rem; will DTRT.
Then div64_u64(a, b) can be div64_u32_rem(a, b, NULL) without
any inlined version containing an actual NULL check.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)