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) >
