Re: [PATCH 4/7] minmax: Simplify signedness check

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Jul 26, 2024 at 12:57:43PM GMT, David Laight wrote:
> From: Lorenzo Stoakes
> > Sent: 26 July 2024 10:44
> >
> > On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote:

[snip]

> > > Christ. This whole series is a nightmare of "add complexity to deal
> > > with stupid issues".
> > >
> > > But the kernel test robot clearly found even more issues.
> > >
> > > I think we need to just go back to the old code. It was stupid and
> > > limited and caused us to have to be more careful about types than was
> > > strictly necessary.
> >
> > The problem is simply reverting reveals that seemingly a _ton_ of code has
> > come to rely on the relaxed conditions.
> >
> > When I went to gather the numbers for my initial report I had to manually
> > fix up every case which was rather painful, and that was just a defconfig +
> > a few extra options. allmodconfig is likely to be a hellscape.
> >
> > I've not dug deep into the ins + outs of this, so forgive me for being
> > vague (Arnd has a far clearer understanding) - but it seems that the
> > majority of the complexity comes from having to absolutely ensure all this
> > works for compile-time constant values.
>
> The problems arise due to some odd uses, not just the requirement for compile-time
> constants for on-stack array sizes.

Odd implies not many, same argument applies.

[snip]

>
> > Arnd had a look through and determined there weren't _too_ many cases where
> > we need this (for instance array sizes).
> >
> > So I wonder whether we can't just vastly simplify this stuff (and reducing
> > the macro expansion hell) for the usual case, and implement something like
> > cmin()/cmax() or whatever for the true-constant cases?
>
> I did do that in a patch set from much earlier in the year.
> But Linus said they'd need to be MIN() and MAX() and that requires changes
> to a few places where those are already defined.

OK, so what's stopping you from doing that?

In order to implement a MIN()/MAX() you'd need to change call sites only
(should be a managable amount), so we can change this too?

I'm concerned that a solution is being proposed here and then handwaved
away...

Unfortunately a revert is no longer possible (I had to fix up 33 call sites
manually just for my defconfig build to compare perf before/after), so if
the intent is to eliminate the complexity, then we need a practical way
forward.

>
> > > But it was also about a million times simpler, and didn't cause build
> > > time regressions.
>
> Just bugs because people did min_t(short, 65536, 128) and didn't expect zero.
>
> It has to be said that the chances of a min(negative_value, unsigned_constant)
> appearing are pretty slim.
> All these tests are there to trap that case.
>
> There is always the option of disabling the tests for 'normal' build, but
> leaving them there for (say) the W=1 builds.
> Then it won't matter as much if the tests slow down the build a little.

Very much NAK disabling tests as a solution! Also leaving them for a build
that's apparently broken... yeah not a fan.

>
> I think I have tried a W=1 build - but there are too many warnings/errors
> from other places to get anywhere.
> A lot are for 'unsigned_var >= 0' in paths that get optimised away.
> The compiler doesn't help!
>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>