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

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

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:
> > Hi Lorenzo,
> >
> > On 9/11/24 5:34 PM, Lorenzo Stoakes wrote:
> > > 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/
> > > [1]:https://lore.kernel.org/lkml/5882b96e-1287-4390-8174-3316d39038ef@lucifer.local/
> > > [2]:https://lore.kernel.org/linux-mm/36aa2cad-1db1-4abf-8dd2-fb20484aabc3@lucifer.local/
> > >
> > > Reported-by: Richard Narron <richard@aaazen.com>
> > > Closes: https://lore.kernel.org/all/4a5321bd-b1f-1832-f0c-cea8694dc5aa@aaazen.com/
> > > Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > 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
>