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.co... [1]:https://lore.kernel.org/lkml/5882b96e-1287-4390-8174-3316d39038ef@lucifer.lo... [2]:https://lore.kernel.org/linux-mm/36aa2cad-1db1-4abf-8dd2-fb20484aabc3@lucife...
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 --- 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) +{ + 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); -- 2.46.0
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.
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...
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) \
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
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.
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...
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) \
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
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.
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...
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) \
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
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
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
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.
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).
regards, dan carpenter
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.
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
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 2 +-
For this part only:
Reviewed-by: Andrew Lunn andrew@lunn.ch
Andrew
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Dan Carpenter -
Hans de Goede -
Lorenzo Stoakes