From: Linus Torvalds <torvalds@linux-foundation.org>
[ Upstream commit dc1c8034e31b14a2e5e212104ec508aec44ce1b9 ]
Now that we no longer have any C constant expression contexts (ie array
size declarations or static initializers) that use min() or max(), we
can simpify the implementation by not having to worry about the result
staying as a C constant expression.
So now we can unconditionally just use temporary variables of the right
type, and get rid of the excessive expansion that used to come from the
use of
__builtin_choose_expr(__is_constexpr(...), ..
to pick the specialized code for constant expressions.
Another expansion simplification is to pass the temporary variables (in
addition to the original expression) to our __types_ok() macro. That
may superficially look like it complicates the macro, but when we only
want the type of the expression, expanding the temporary variable names
is much simpler and smaller than expanding the potentially complicated
original expression.
As a result, on my machine, doing a
$ time make drivers/staging/media/atomisp/pci/isp/kernels/ynr/ynr_1.0/ia_css_ynr.host.i
goes from
real 0m16.621s
user 0m15.360s
sys 0m1.221s
to
real 0m2.532s
user 0m2.091s
sys 0m0.452s
because the token expansion goes down dramatically.
In particular, the longest line expansion (which was line 71 of that
'ia_css_ynr.host.c' file) shrinks from 23,338kB (yes, 23MB for one
single line) to "just" 1,444kB (now "only" 1.4MB).
And yes, that line is still the line from hell, because it's doing
multiple levels of "min()/max()" expansion thanks to some of them being
hidden inside the uDIGIT_FITTING() macro.
Lorenzo has a nice cleanup patch that makes that driver use inline
functions instead of macros for sDIGIT_FITTING() and uDIGIT_FITTING(),
which will fix that line once and for all, but the 16-fold reduction in
this case does show why we need to simplify these helpers.
Cc: David Laight <David.Laight@aculab.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
include/linux/minmax.h | 43 ++++++++++++++++++++----------------------
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index fc384714da45..e3e4353df983 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -35,10 +35,10 @@
#define __is_noneg_int(x) \
(__builtin_choose_expr(__is_constexpr(x) && __is_signed(x), x, -1) >= 0)
-#define __types_ok(x, y) \
- (__is_signed(x) == __is_signed(y) || \
- __is_signed((x) + 0) == __is_signed((y) + 0) || \
- __is_noneg_int(x) || __is_noneg_int(y))
+#define __types_ok(x, y, ux, uy) \
+ (__is_signed(ux) == __is_signed(uy) || \
+ __is_signed((ux) + 0) == __is_signed((uy) + 0) || \
+ __is_noneg_int(x) || __is_noneg_int(y))
#define __cmp_op_min <
#define __cmp_op_max >
@@ -51,34 +51,31 @@
#define __cmp_once(op, type, x, y) \
__cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
-#define __careful_cmp_once(op, x, y) ({ \
- static_assert(__types_ok(x, y), \
+#define __careful_cmp_once(op, x, y, ux, uy) ({ \
+ __auto_type ux = (x); __auto_type uy = (y); \
+ static_assert(__types_ok(x, y, ux, uy), \
#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
- __cmp_once(op, __auto_type, x, y); })
+ __cmp(op, ux, uy); })
-#define __careful_cmp(op, x, y) \
- __builtin_choose_expr(__is_constexpr((x) - (y)), \
- __cmp(op, x, y), __careful_cmp_once(op, x, y))
+#define __careful_cmp(op, x, y) \
+ __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
#define __clamp(val, lo, hi) \
((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
-#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({ \
- typeof(val) unique_val = (val); \
- typeof(lo) unique_lo = (lo); \
- typeof(hi) unique_hi = (hi); \
+#define __clamp_once(val, lo, hi, uval, ulo, uhi) ({ \
+ __auto_type uval = (val); \
+ __auto_type ulo = (lo); \
+ __auto_type uhi = (hi); \
static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
(lo) <= (hi), true), \
"clamp() low limit " #lo " greater than high limit " #hi); \
- static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
- __clamp(unique_val, unique_lo, unique_hi); })
-
-#define __careful_clamp(val, lo, hi) ({ \
- __builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
- __clamp(val, lo, hi), \
- __clamp_once(val, lo, hi, __UNIQUE_ID(__val), \
- __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
+ static_assert(__types_ok(uval, lo, uval, ulo), "clamp() 'lo' signedness error"); \
+ static_assert(__types_ok(uval, hi, uval, uhi), "clamp() 'hi' signedness error"); \
+ __clamp(uval, ulo, uhi); })
+
+#define __careful_clamp(val, lo, hi) \
+ __clamp_once(val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
/**
* min - return minimum of two values of the same or compatible types
--
2.47.3On Mon, Sep 29, 2025 at 06:33:48PM +0000, Eliav Farber wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
>
> [ Upstream commit dc1c8034e31b14a2e5e212104ec508aec44ce1b9 ]
>
> Now that we no longer have any C constant expression contexts (ie array
> size declarations or static initializers) that use min() or max(), we
> can simpify the implementation by not having to worry about the result
> staying as a C constant expression.
>
> So now we can unconditionally just use temporary variables of the right
> type, and get rid of the excessive expansion that used to come from the
> use of
>
> __builtin_choose_expr(__is_constexpr(...), ..
>
> to pick the specialized code for constant expressions.
>
> Another expansion simplification is to pass the temporary variables (in
> addition to the original expression) to our __types_ok() macro. That
> may superficially look like it complicates the macro, but when we only
> want the type of the expression, expanding the temporary variable names
> is much simpler and smaller than expanding the potentially complicated
> original expression.
>
> As a result, on my machine, doing a
>
> $ time make drivers/staging/media/atomisp/pci/isp/kernels/ynr/ynr_1.0/ia_css_ynr.host.i
>
> goes from
>
> real 0m16.621s
> user 0m15.360s
> sys 0m1.221s
>
> to
>
> real 0m2.532s
> user 0m2.091s
> sys 0m0.452s
>
> because the token expansion goes down dramatically.
>
> In particular, the longest line expansion (which was line 71 of that
> 'ia_css_ynr.host.c' file) shrinks from 23,338kB (yes, 23MB for one
> single line) to "just" 1,444kB (now "only" 1.4MB).
>
> And yes, that line is still the line from hell, because it's doing
> multiple levels of "min()/max()" expansion thanks to some of them being
> hidden inside the uDIGIT_FITTING() macro.
>
> Lorenzo has a nice cleanup patch that makes that driver use inline
> functions instead of macros for sDIGIT_FITTING() and uDIGIT_FITTING(),
> which will fix that line once and for all, but the 16-fold reduction in
> this case does show why we need to simplify these helpers.
>
> Cc: David Laight <David.Laight@aculab.com>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> include/linux/minmax.h | 43 ++++++++++++++++++++----------------------
> 1 file changed, 20 insertions(+), 23 deletions(-)
This change breaks the build in drivers/md/ :
In file included from ./include/linux/container_of.h:5,
from ./include/linux/list.h:5,
from ./include/linux/wait.h:7,
from ./include/linux/mempool.h:8,
from ./include/linux/bio.h:8,
from drivers/md/dm-bio-record.h:10,
from drivers/md/dm-integrity.c:9:
drivers/md/dm-integrity.c: In function ‘integrity_metadata’:
drivers/md/dm-integrity.c:131:105: error: ISO C90 forbids variable length array ‘checksums_onstack’ [-Werror=vla]
131 | #define MAX_TAG_SIZE (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK]))
| ^~~~~~~~~~~~~
./include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
./include/linux/minmax.h:56:9: note: in expansion of macro ‘static_assert’
56 | static_assert(__types_ok(x, y, ux, uy), \
| ^~~~~~~~~~~~~
./include/linux/minmax.h:41:31: note: in expansion of macro ‘__is_noneg_int’
41 | __is_noneg_int(x) || __is_noneg_int(y))
| ^~~~~~~~~~~~~~
./include/linux/minmax.h:56:23: note: in expansion of macro ‘__types_ok’
56 | static_assert(__types_ok(x, y, ux, uy), \
| ^~~~~~~~~~
./include/linux/minmax.h:61:9: note: in expansion of macro ‘__careful_cmp_once’
61 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
| ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:92:25: note: in expansion of macro ‘__careful_cmp’
92 | #define max(x, y) __careful_cmp(max, x, y)
| ^~~~~~~~~~~~~
drivers/md/dm-integrity.c:1797:40: note: in expansion of macro ‘max’
1797 | char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
| ^~~
drivers/md/dm-integrity.c:131:89: note: in expansion of macro ‘offsetof’
131 | #define MAX_TAG_SIZE (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK]))
| ^~~~~~~~
drivers/md/dm-integrity.c:1797:73: note: in expansion of macro ‘MAX_TAG_SIZE’
1797 | char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)];
| ^~~~~~~~~~~~
So I'll stop here on this series.
After the next release, can you rebase the series and resend the remaining ones after they are fixed up to build properly?
thanks,
greg k-h
> On Mon, Sep 29, 2025 at 06:33:48PM +0000, Eliav Farber wrote: > > From: Linus Torvalds <torvalds@linux-foundation.org> > > > > [ Upstream commit dc1c8034e31b14a2e5e212104ec508aec44ce1b9 ] > > > > Now that we no longer have any C constant expression contexts (ie array > > size declarations or static initializers) that use min() or max(), we > > can simpify the implementation by not having to worry about the result > > staying as a C constant expression. > > > > So now we can unconditionally just use temporary variables of the right > > type, and get rid of the excessive expansion that used to come from the > > use of > > > > __builtin_choose_expr(__is_constexpr(...), .. > > > > to pick the specialized code for constant expressions. > > > > Another expansion simplification is to pass the temporary variables (in > > addition to the original expression) to our __types_ok() macro. That > > may superficially look like it complicates the macro, but when we only > > want the type of the expression, expanding the temporary variable names > > is much simpler and smaller than expanding the potentially complicated > > original expression. > > > > As a result, on my machine, doing a > > > > $ time make drivers/staging/media/atomisp/pci/isp/kernels/ynr/ynr_1.0/ia_css_ynr.host.i > > > > goes from > > > > real 0m16.621s > > user 0m15.360s > > sys 0m1.221s > > > > to > > > > real 0m2.532s > > user 0m2.091s > > sys 0m0.452s > > > > because the token expansion goes down dramatically. > > > > In particular, the longest line expansion (which was line 71 of that > > 'ia_css_ynr.host.c' file) shrinks from 23,338kB (yes, 23MB for one > > single line) to "just" 1,444kB (now "only" 1.4MB). > > > > And yes, that line is still the line from hell, because it's doing > > multiple levels of "min()/max()" expansion thanks to some of them being > > hidden inside the uDIGIT_FITTING() macro. > > > > Lorenzo has a nice cleanup patch that makes that driver use inline > > functions instead of macros for sDIGIT_FITTING() and uDIGIT_FITTING(), > > which will fix that line once and for all, but the 16-fold reduction in > > this case does show why we need to simplify these helpers. > > > > Cc: David Laight <David.Laight@aculab.com> > > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Eliav Farber <farbere@amazon.com> > > --- > > include/linux/minmax.h | 43 ++++++++++++++++++++---------------------- > > 1 file changed, 20 insertions(+), 23 deletions(-) > > This change breaks the build in drivers/md/ : > > In file included from ./include/linux/container_of.h:5, > from ./include/linux/list.h:5, > from ./include/linux/wait.h:7, > from ./include/linux/mempool.h:8, > from ./include/linux/bio.h:8, > from drivers/md/dm-bio-record.h:10, > from drivers/md/dm-integrity.c:9: > drivers/md/dm-integrity.c: In function ‘integrity_metadata’: > drivers/md/dm-integrity.c:131:105: error: ISO C90 forbids variable length array ‘checksums_onstack’ [-Werror=vla] > 131 | #define MAX_TAG_SIZE (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK])) > | ^~~~~~~~~~~~~ >./include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ > 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) > | ^~~~ >./include/linux/minmax.h:56:9: note: in expansion of macro ‘static_assert’ > 56 | static_assert(__types_ok(x, y, ux, uy), \ > | ^~~~~~~~~~~~~ >./include/linux/minmax.h:41:31: note: in expansion of macro ‘__is_noneg_int’ > 41 | __is_noneg_int(x) || __is_noneg_int(y)) > | ^~~~~~~~~~~~~~ >./include/linux/minmax.h:56:23: note: in expansion of macro ‘__types_ok’ > 56 | static_assert(__types_ok(x, y, ux, uy), \ > | ^~~~~~~~~~ >./include/linux/minmax.h:61:9: note: in expansion of macro ‘__careful_cmp_once’ > 61 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) > | ^~~~~~~~~~~~~~~~~~ >./include/linux/minmax.h:92:25: note: in expansion of macro ‘__careful_cmp’ > 92 | #define max(x, y) __careful_cmp(max, x, y) > | ^~~~~~~~~~~~~ > drivers/md/dm-integrity.c:1797:40: note: in expansion of macro ‘max’ > 1797 | char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; > | ^~~ > drivers/md/dm-integrity.c:131:89: note: in expansion of macro ‘offsetof’ > 131 | #define MAX_TAG_SIZE (JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR - offsetof(struct journal_entry, last_bytes[MAX_SECTORS_PER_BLOCK])) > | ^~~~~~~~ > drivers/md/dm-integrity.c:1797:73: note: in expansion of macro ‘MAX_TAG_SIZE’ > 1797 | char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; > | ^~~~~~~~~~~~ > > > So I'll stop here on this series. > > After the next release, can you rebase the series and resend the remaining ones after they are fixed up to build properly? Sure. I see the problem. In the integrity_metadata() function it should be: char checksums_onstack[MAX(HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; instead of: char checksums_onstack[max((size_t)HASH_MAX_DIGESTSIZE, MAX_TAG_SIZE)]; --- Regards, Eliav
© 2016 - 2025 Red Hat, Inc.