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

Commit Message

Lorenzo Stoakes Sept. 11, 2024, 3:34 p.m. UTC

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/[e-mail]/
[1]:https://lore.kernel.org/lkml/[e-mail]/
[2]:https://lore.kernel.org/linux-mm/[e-mail]/

Reported-by: Richard Narron <[e-mail]>
Closes: https://lore.kernel.org/all/[e-mail]/
Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
Cc: [e-mail]
Signed-off-by: Lorenzo Stoakes <[e-mail]>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-
 .../staging/media/atomisp/pci/sh_css_frac.h   | 26 ++++++++++++++-----
 include/linux/skbuff.h                        |  6 ++++-
 3 files changed, 25 insertions(+), 9 deletions(-)

--
2.46.0

Comments

Hans de Goede Sept. 11, 2024, 4:24 p.m. UTC | #1

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/[e-mail]/
> [1]:https://lore.kernel.org/lkml/[e-mail]/
> [2]:https://lore.kernel.org/linux-mm/[e-mail]/
> 
> Reported-by: Richard Narron <[e-mail]>
> Closes: https://lore.kernel.org/all/[e-mail]/
> Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> Cc: [e-mail]
> Signed-off-by: Lorenzo Stoakes <[e-mail]>

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

So I would prefer this to be split into 3 patches.

One review comment for the atomisp bits inline / below.

> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-
>  .../staging/media/atomisp/pci/sh_css_frac.h   | 26 ++++++++++++++-----
>  include/linux/skbuff.h                        |  6 ++++-
>  3 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index e809f91c08fb..8b431f90efc3 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -23,7 +23,7 @@
>  /* The PacketOffset field is measured in units of 32 bytes and is 3 bits wide,
>   * so the maximum offset is 7 * 32 = 224
>   */
> -#define MVPP2_SKB_HEADROOM	min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
> +#define MVPP2_SKB_HEADROOM	clamp_t(int, XDP_PACKET_HEADROOM, NET_SKB_PAD, 224)
> 
>  #define MVPP2_XDP_PASS		0
>  #define MVPP2_XDP_DROPPED	BIT(0)
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> index b90b5b330dfa..a973394c5bc0 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> @@ -32,12 +32,24 @@
>  #define uISP_VAL_MAX		      ((unsigned int)((1 << uISP_REG_BIT) - 1))
> 
>  /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
> -#define sDIGIT_FITTING(v, a, b) \
> -	min_t(int, max_t(int, (((v) >> sSHIFT) >> max(sFRACTION_BITS_FITTING(a) - (b), 0)), \
> -	  sISP_VAL_MIN), sISP_VAL_MAX)
> -#define uDIGIT_FITTING(v, a, b) \
> -	min((unsigned int)max((unsigned)(((v) >> uSHIFT) \
> -	>> max((int)(uFRACTION_BITS_FITTING(a) - (b)), 0)), \
> -	  uISP_VAL_MIN), uISP_VAL_MAX)
> +static inline int sDIGIT_FITTING(short v, int a, int b)
> +{

drivers/staging/media/atomisp/pci/isp/kernels/s3a/s3a_1.0/ia_css_s3a.host.c

calls this with ia_css_3a_config.af_fir1_coef / .af_fir2_coef
as first argument those are of the ia_css_s0_15 type which is:

/* Signed fixed point value, 0 integer bits, 15 fractional bits */
typedef s32 ia_css_s0_15;

please replace the "short v" with "int v" 

I think that you can then also replace clamp_t() with clamp()


> +	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
> +
> +	v >>= sSHIFT;
> +	v >>= fit_shift > 0 ? fit_shift : 0;
> +
> +	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
> +}
> +
> +static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
> +{
> +	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
> +
> +	v >>= uSHIFT;
> +	v >>= fit_shift > 0 ? fit_shift : 0;
> +
> +	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
> +}

Regular clamp() should work here ? all parameters are already
unsigned ints.

Regards,

Hans




> 
>  #endif /* __SH_CSS_FRAC_H */
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 29c3ea5b6e93..d53b296df504 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3164,7 +3164,11 @@ static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len)
>   * NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
>   */
>  #ifndef NET_SKB_PAD
> -#define NET_SKB_PAD	max(32, L1_CACHE_BYTES)
> +#if L1_CACHE_BYTES < 32
> +#define NET_SKB_PAD	32
> +#else
> +#define NET_SKB_PAD	L1_CACHE_BYTES
> +#endif
>  #endif
> 
>  int ___pskb_trim(struct sk_buff *skb, unsigned int len);
> --
> 2.46.0
>

Lorenzo Stoakes Sept. 11, 2024, 4:37 p.m. UTC | #2

On Wed, Sep 11, 2024 at 06:24:54PM GMT, 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/[e-mail]/
> > [1]:https://lore.kernel.org/lkml/[e-mail]/
> > [2]:https://lore.kernel.org/linux-mm/[e-mail]/
> >
> > Reported-by: Richard Narron <[e-mail]>
> > Closes: https://lore.kernel.org/all/[e-mail]/
> > Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> > Cc: [e-mail]
> > Signed-off-by: Lorenzo Stoakes <[e-mail]>
>
> 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...
>
> So I would prefer this to be split into 3 patches.

Well, I was doing this as a favour to Richard between other work so put
this together quickly, but you're right this is going to be a pain to
backport/revert if issues so absolutely - will do.

Since this is a hotfix I'm going to risk annoying people and shoot out
a v2 on same day as v1. Sorry in advance.

>
> One review comment for the atomisp bits inline / below.
>
> > ---
> >  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-
> >  .../staging/media/atomisp/pci/sh_css_frac.h   | 26 ++++++++++++++-----
> >  include/linux/skbuff.h                        |  6 ++++-
> >  3 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > index e809f91c08fb..8b431f90efc3 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > @@ -23,7 +23,7 @@
> >  /* The PacketOffset field is measured in units of 32 bytes and is 3 bits wide,
> >   * so the maximum offset is 7 * 32 = 224
> >   */
> > -#define MVPP2_SKB_HEADROOM	min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
> > +#define MVPP2_SKB_HEADROOM	clamp_t(int, XDP_PACKET_HEADROOM, NET_SKB_PAD, 224)
> >
> >  #define MVPP2_XDP_PASS		0
> >  #define MVPP2_XDP_DROPPED	BIT(0)
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> > index b90b5b330dfa..a973394c5bc0 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
> > @@ -32,12 +32,24 @@
> >  #define uISP_VAL_MAX		      ((unsigned int)((1 << uISP_REG_BIT) - 1))
> >
> >  /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
> > -#define sDIGIT_FITTING(v, a, b) \
> > -	min_t(int, max_t(int, (((v) >> sSHIFT) >> max(sFRACTION_BITS_FITTING(a) - (b), 0)), \
> > -	  sISP_VAL_MIN), sISP_VAL_MAX)
> > -#define uDIGIT_FITTING(v, a, b) \
> > -	min((unsigned int)max((unsigned)(((v) >> uSHIFT) \
> > -	>> max((int)(uFRACTION_BITS_FITTING(a) - (b)), 0)), \
> > -	  uISP_VAL_MIN), uISP_VAL_MAX)
> > +static inline int sDIGIT_FITTING(short v, int a, int b)
> > +{
>
> drivers/staging/media/atomisp/pci/isp/kernels/s3a/s3a_1.0/ia_css_s3a.host.c
>
> calls this with ia_css_3a_config.af_fir1_coef / .af_fir2_coef
> as first argument those are of the ia_css_s0_15 type which is:
>
> /* Signed fixed point value, 0 integer bits, 15 fractional bits */
> typedef s32 ia_css_s0_15;
>
> please replace the "short v" with "int v"

Yeah I think you're right, it's odd, because it seems that the shift value
and the comments implies that this is a short, but perhaps it's more so
that values are shifted as to obtain 16 bits of precision.

>
> I think that you can then also replace clamp_t() with clamp()

The use of clamp_t() is to avoid egregious macro expansion in
clamp(). After the series improving min/max the clamp() is probably
equivalent. But in 5.15 it will likely not be. So this is, in line with the
purpose of this change, I believe necesasry.

>
>
> > +	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
> > +
> > +	v >>= sSHIFT;
> > +	v >>= fit_shift > 0 ? fit_shift : 0;
> > +
> > +	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
> > +}
> > +
> > +static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
> > +{
> > +	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
> > +
> > +	v >>= uSHIFT;
> > +	v >>= fit_shift > 0 ? fit_shift : 0;
> > +
> > +	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
> > +}
>
> Regular clamp() should work here ? all parameters are already
> unsigned ints.

See above.

>
> Regards,
>
> Hans
>
>
>
>
> >
> >  #endif /* __SH_CSS_FRAC_H */
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 29c3ea5b6e93..d53b296df504 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3164,7 +3164,11 @@ static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len)
> >   * NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
> >   */
> >  #ifndef NET_SKB_PAD
> > -#define NET_SKB_PAD	max(32, L1_CACHE_BYTES)
> > +#if L1_CACHE_BYTES < 32
> > +#define NET_SKB_PAD	32
> > +#else
> > +#define NET_SKB_PAD	L1_CACHE_BYTES
> > +#endif
> >  #endif
> >
> >  int ___pskb_trim(struct sk_buff *skb, unsigned int len);
> > --
> > 2.46.0
> >
>

Hans de Goede Sept. 11, 2024, 4:44 p.m. UTC | #3

Hi,

On 9/11/24 6:37 PM, Lorenzo Stoakes wrote:
> On Wed, Sep 11, 2024 at 06:24:54PM GMT, 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/[e-mail]/
>>> [1]:https://lore.kernel.org/lkml/[e-mail]/
>>> [2]:https://lore.kernel.org/linux-mm/[e-mail]/
>>>
>>> Reported-by: Richard Narron <[e-mail]>
>>> Closes: https://lore.kernel.org/all/[e-mail]/
>>> Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
>>> Cc: [e-mail]
>>> Signed-off-by: Lorenzo Stoakes <[e-mail]>
>>
>> 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...
>>
>> So I would prefer this to be split into 3 patches.
> 
> Well, I was doing this as a favour to Richard between other work so put
> this together quickly, but you're right this is going to be a pain to
> backport/revert if issues so absolutely - will do.
> 
> Since this is a hotfix I'm going to risk annoying people and shoot out
> a v2 on same day as v1. Sorry in advance.
> 
>>
>> One review comment for the atomisp bits inline / below.
>>
>>> ---
>>>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-
>>>  .../staging/media/atomisp/pci/sh_css_frac.h   | 26 ++++++++++++++-----
>>>  include/linux/skbuff.h                        |  6 ++++-
>>>  3 files changed, 25 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> index e809f91c08fb..8b431f90efc3 100644
>>> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
>>> @@ -23,7 +23,7 @@
>>>  /* The PacketOffset field is measured in units of 32 bytes and is 3 bits wide,
>>>   * so the maximum offset is 7 * 32 = 224
>>>   */
>>> -#define MVPP2_SKB_HEADROOM	min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
>>> +#define MVPP2_SKB_HEADROOM	clamp_t(int, XDP_PACKET_HEADROOM, NET_SKB_PAD, 224)
>>>
>>>  #define MVPP2_XDP_PASS		0
>>>  #define MVPP2_XDP_DROPPED	BIT(0)
>>> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> index b90b5b330dfa..a973394c5bc0 100644
>>> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
>>> @@ -32,12 +32,24 @@
>>>  #define uISP_VAL_MAX		      ((unsigned int)((1 << uISP_REG_BIT) - 1))
>>>
>>>  /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
>>> -#define sDIGIT_FITTING(v, a, b) \
>>> -	min_t(int, max_t(int, (((v) >> sSHIFT) >> max(sFRACTION_BITS_FITTING(a) - (b), 0)), \
>>> -	  sISP_VAL_MIN), sISP_VAL_MAX)
>>> -#define uDIGIT_FITTING(v, a, b) \
>>> -	min((unsigned int)max((unsigned)(((v) >> uSHIFT) \
>>> -	>> max((int)(uFRACTION_BITS_FITTING(a) - (b)), 0)), \
>>> -	  uISP_VAL_MIN), uISP_VAL_MAX)
>>> +static inline int sDIGIT_FITTING(short v, int a, int b)
>>> +{
>>
>> drivers/staging/media/atomisp/pci/isp/kernels/s3a/s3a_1.0/ia_css_s3a.host.c
>>
>> calls this with ia_css_3a_config.af_fir1_coef / .af_fir2_coef
>> as first argument those are of the ia_css_s0_15 type which is:
>>
>> /* Signed fixed point value, 0 integer bits, 15 fractional bits */
>> typedef s32 ia_css_s0_15;
>>
>> please replace the "short v" with "int v"
> 
> Yeah I think you're right, it's odd, because it seems that the shift value
> and the comments implies that this is a short, but perhaps it's more so
> that values are shifted as to obtain 16 bits of precision.
> 
>>
>> I think that you can then also replace clamp_t() with clamp()
> 
> The use of clamp_t() is to avoid egregious macro expansion in
> clamp(). After the series improving min/max the clamp() is probably
> equivalent. But in 5.15 it will likely not be. So this is, in line with the
> purpose of this change, I believe necesasry.

Ok fair enough.

Regards,

Hans

Andrew Lunn Sept. 11, 2024, 4:57 p.m. UTC | #4

> > I think that you can then also replace clamp_t() with clamp()
> 
> The use of clamp_t() is to avoid egregious macro expansion in
> clamp(). After the series improving min/max the clamp() is probably
> equivalent. But in 5.15 it will likely not be. So this is, in line with the
> purpose of this change, I believe necesasry.

Maybe that should be in the commit message?

	Andrew

Lorenzo Stoakes Sept. 11, 2024, 5 p.m. UTC | #5

On Wed, Sep 11, 2024 at 06:57:47PM GMT, Andrew Lunn wrote:
> > > I think that you can then also replace clamp_t() with clamp()
> >
> > The use of clamp_t() is to avoid egregious macro expansion in
> > clamp(). After the series improving min/max the clamp() is probably
> > equivalent. But in 5.15 it will likely not be. So this is, in line with the
> > purpose of this change, I believe necesasry.
>
> Maybe that should be in the commit message?

message-s :)

Yup, I'm putting that in.

>
> 	Andrew

Andrew Lunn Sept. 11, 2024, 5:01 p.m. UTC | #6

>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  2 +-

For this part only:

Reviewed-by: Andrew Lunn <[e-mail]>

    Andrew

Dan Carpenter Sept. 11, 2024, 5:25 p.m. UTC | #7

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/[e-mail]/
> > [1]:https://lore.kernel.org/lkml/[e-mail]/
> > [2]:https://lore.kernel.org/linux-mm/[e-mail]/
> > 
> > Reported-by: Richard Narron <[e-mail]>
> > Closes: https://lore.kernel.org/all/[e-mail]/
> > Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> > Cc: [e-mail]
> > Signed-off-by: Lorenzo Stoakes <[e-mail]>
> 
> 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).

regards,
dan carpenter

Lorenzo Stoakes Sept. 11, 2024, 5:36 p.m. UTC | #8

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/[e-mail]/
> > > [1]:https://lore.kernel.org/lkml/[e-mail]/
> > > [2]:https://lore.kernel.org/linux-mm/[e-mail]/
> > >
> > > Reported-by: Richard Narron <[e-mail]>
> > > Closes: https://lore.kernel.org/all/[e-mail]/
> > > Fixes: 867046cc7027 ("minmax: relax check to allow comparison between unsigned arguments and signed constants")
> > > Cc: [e-mail]
> > > Signed-off-by: Lorenzo Stoakes <[e-mail]>
> >
> > 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
>

Patch

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index e809f91c08fb..8b431f90efc3 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -23,7 +23,7 @@ 
 /* The PacketOffset field is measured in units of 32 bytes and is 3 bits wide,
  * so the maximum offset is 7 * 32 = 224
  */
-#define MVPP2_SKB_HEADROOM	min(max(XDP_PACKET_HEADROOM, NET_SKB_PAD), 224)
+#define MVPP2_SKB_HEADROOM	clamp_t(int, XDP_PACKET_HEADROOM, NET_SKB_PAD, 224)

 #define MVPP2_XDP_PASS		0
 #define MVPP2_XDP_DROPPED	BIT(0)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h
index b90b5b330dfa..a973394c5bc0 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_frac.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h
@@ -32,12 +32,24 @@ 
 #define uISP_VAL_MAX		      ((unsigned int)((1 << uISP_REG_BIT) - 1))

 /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
-#define sDIGIT_FITTING(v, a, b) \
-	min_t(int, max_t(int, (((v) >> sSHIFT) >> max(sFRACTION_BITS_FITTING(a) - (b), 0)), \
-	  sISP_VAL_MIN), sISP_VAL_MAX)
-#define uDIGIT_FITTING(v, a, b) \
-	min((unsigned int)max((unsigned)(((v) >> uSHIFT) \
-	>> max((int)(uFRACTION_BITS_FITTING(a) - (b)), 0)), \
-	  uISP_VAL_MIN), uISP_VAL_MAX)
+static inline int sDIGIT_FITTING(short v, int a, int b)
+{
+	int fit_shift = sFRACTION_BITS_FITTING(a) - b;
+
+	v >>= sSHIFT;
+	v >>= fit_shift > 0 ? fit_shift : 0;
+
+	return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX);
+}
+
+static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b)
+{
+	int fit_shift = uFRACTION_BITS_FITTING(a) - b;
+
+	v >>= uSHIFT;
+	v >>= fit_shift > 0 ? fit_shift : 0;
+
+	return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX);
+}

 #endif /* __SH_CSS_FRAC_H */
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 29c3ea5b6e93..d53b296df504 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3164,7 +3164,11 @@  static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len)
  * NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
  */
 #ifndef NET_SKB_PAD
-#define NET_SKB_PAD	max(32, L1_CACHE_BYTES)
+#if L1_CACHE_BYTES < 32
+#define NET_SKB_PAD	32
+#else
+#define NET_SKB_PAD	L1_CACHE_BYTES
+#endif
 #endif

 int ___pskb_trim(struct sk_buff *skb, unsigned int len);