Re: [PATCH hotfix 6.11] minmax: reduce egregious min/max macro expansion

On Wed, Sep 11, 2024 at 08:25:33PM GMT, Dan Carpenter wrote:

On Wed, Sep 11, 2024 at 06:24:54PM +0200, Hans de Goede wrote:

quoted

Hi Lorenzo,

On 9/11/24 5:34 PM, Lorenzo Stoakes wrote:

quoted

Avoid nested min()/max() which results in egregious macro expansion.

This issue was introduced by commit 867046cc7027 ("minmax: relax check to
allow comparison between unsigned arguments and signed constants") [2].

Work has been done to address the issue of egregious min()/max() macro
expansion in commit 22f546873149 ("minmax: improve macro expansion and type
checking") and related, however it appears that some issues remain on more
tightly constrained systems.

Adjust a few known-bad cases of deeply nested macros to avoid doing so to
mitigate this. Porting the patch first proposed in [1] to Linus's tree.

Running an allmodconfig build using the methodology described in [2] we
observe a 35 MiB reduction in generated code.

The difference is much more significant prior to recent minmax fixes which
were not backported. As per [1] prior these the reduction is more like 200
MiB.

This resolves an issue with slackware 15.0 32-bit compilation as reported
by Richard Narron.

Presumably the min/max fixups would be difficult to backport, this patch
should be easier and fix's Richard's problem in 5.15.

[0]:https://lore.kernel.org/all/b97faef60ad24922b530241c5d7c933c@AcuMS.aculab.com/ (local)
[1]:https://lore.kernel.org/lkml/5882b96e-1287-4390-8174-3316d39038ef@lucifer.local/ (local)
[2]:https://lore.kernel.org/linux-mm/36aa2cad-1db1-4abf-8dd2-fb20484aabc3@lucifer.local/ (local)

Reported-by: Richard Narron <redacted>
Closes: https://lore.kernel.org/all/4a5321bd-b1f-1832-f0c-cea8694dc5aa@aaazen.com/ (local)
Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
Cc: stable@vger.kernel.org
Signed-off-by: Lorenzo Stoakes <redacted>

Thank you for your patch.

I must say that I'm not a fan of that this is patching 3 totally
unrelated files here in a single patch.

This is e.g. going to be a problem if we need to revert one of
the changes because of regressions...

These kinds of thing also complicates backporting to stable.  The stable kernel
developers like whole, unmodified patches.  So if we have to fix something in
sDIGIT_FITTING() then we'd want to pull this back instead of re-writing the fix
on top of the original define (unmodified patches).  But now we have to backport
the chunk which changes mvpp2 as well (whole patches).

Sure absolutely, as I said to Hans I did it all as one as I wanted to get
this out quickly as a favour to Richard, but this was a mistake, very
obviously it's much easier to have these separate.

About to send out a v2 with this done. Cheers!

regards,
dan carpenter