[PATCH v2 07/19 5.15.y] minmax: simplify and clarify min_t()/max_t() implementation

Eliav Farber posted 19 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 07/19 5.15.y] minmax: simplify and clarify min_t()/max_t() implementation
Posted by Eliav Farber 2 months, 2 weeks ago
From: Linus Torvalds <torvalds@linux-foundation.org>

[ Upstream commit 017fa3e89187848fd056af757769c9e66ac3e93d ]

This simplifies the min_t() and max_t() macros by no longer making them
work in the context of a C constant expression.

That means that you can no longer use them for static initializers or
for array sizes in type definitions, but there were only a couple of
such uses, and all of them were converted (famous last words) to use
MIN_T/MAX_T instead.

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>
---
V1 -> V2:
Use `[ Upstream commit <HASH> ]` instead of `commit <HASH> upstream.`
like in all other patches.

 include/linux/minmax.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index a7ef65f78933..9c2848abc804 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -45,17 +45,20 @@
 
 #define __cmp(op, x, y)	((x) __cmp_op_##op (y) ? (x) : (y))
 
-#define __cmp_once(op, x, y, unique_x, unique_y) ({	\
-	typeof(x) unique_x = (x);			\
-	typeof(y) unique_y = (y);			\
+#define __cmp_once_unique(op, type, x, y, ux, uy) \
+	({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
+
+#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),			\
 		#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
-	__cmp(op, unique_x, unique_y); })
+	__cmp_once(op, __auto_type, x, y); })
 
 #define __careful_cmp(op, x, y)					\
 	__builtin_choose_expr(__is_constexpr((x) - (y)),	\
-		__cmp(op, x, y),				\
-		__cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
+		__cmp(op, x, y), __careful_cmp_once(op, x, y))
 
 #define __clamp(val, lo, hi)	\
 	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
@@ -158,7 +161,7 @@
  * @x: first value
  * @y: second value
  */
-#define min_t(type, x, y)	__careful_cmp(min, (type)(x), (type)(y))
+#define min_t(type, x, y) __cmp_once(min, type, x, y)
 
 /**
  * max_t - return maximum of two values, using the specified type
@@ -166,7 +169,7 @@
  * @x: first value
  * @y: second value
  */
-#define max_t(type, x, y)	__careful_cmp(max, (type)(x), (type)(y))
+#define max_t(type, x, y) __cmp_once(max, type, x, y)
 
 /*
  * Do not check the array parameter using __must_be_array().
-- 
2.47.3
Re: [PATCH v2 07/19 5.15.y] minmax: simplify and clarify min_t()/max_t() implementation
Posted by Greg KH 2 months, 1 week ago
On Fri, Oct 03, 2025 at 12:59:54PM +0000, Eliav Farber wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> [ Upstream commit 017fa3e89187848fd056af757769c9e66ac3e93d ]
> 
> This simplifies the min_t() and max_t() macros by no longer making them
> work in the context of a C constant expression.
> 
> That means that you can no longer use them for static initializers or
> for array sizes in type definitions, but there were only a couple of
> such uses, and all of them were converted (famous last words) to use
> MIN_T/MAX_T instead.
> 
> 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>

Eliav, your testing infrastructure needs some work, this patch breaks
the build on this kernel tree:

In file included from ./include/linux/kernel.h:16,
                 from ./include/linux/list.h:9,
                 from ./include/linux/wait.h:7,
                 from ./include/linux/wait_bit.h:8,
                 from ./include/linux/fs.h:6,
                 from fs/erofs/internal.h:10,
                 from fs/erofs/zdata.h:9,
                 from fs/erofs/zdata.c:6:
fs/erofs/zdata.c: In function ‘z_erofs_decompress_pcluster’:
fs/erofs/zdata.h:185:61: error: ISO C90 forbids variable length array ‘pages_onstack’ [-Werror=vla]
  185 |         min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
      |                                                             ^~~~
./include/linux/minmax.h:49:23: note: in definition of macro ‘__cmp_once_unique’
   49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
      |                       ^
./include/linux/minmax.h:164:27: note: in expansion of macro ‘__cmp_once’
  164 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
      |                           ^~~~~~~~~~
fs/erofs/zdata.h:185:9: note: in expansion of macro ‘min_t’
  185 |         min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
      |         ^~~~~
fs/erofs/zdata.c:847:36: note: in expansion of macro ‘Z_EROFS_VMAP_ONSTACK_PAGES’
  847 |         struct page *pages_onstack[Z_EROFS_VMAP_ONSTACK_PAGES];
      |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


I'll drop this whole series, please do a bit more testing before sending
out a new version.

thanks,

greg k-h
Re: [PATCH v2 07/19 5.15.y] minmax: simplify and clarify min_t()/max_t() implementation
Posted by David Laight 2 months, 1 week ago
On Mon, 6 Oct 2025 12:47:45 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

(I've had to trim the 'To' list to send this...)

> On Fri, Oct 03, 2025 at 12:59:54PM +0000, Eliav Farber wrote:
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > [ Upstream commit 017fa3e89187848fd056af757769c9e66ac3e93d ]
> > 
> > This simplifies the min_t() and max_t() macros by no longer making them
> > work in the context of a C constant expression.
> > 
> > That means that you can no longer use them for static initializers or
> > for array sizes in type definitions, but there were only a couple of
> > such uses, and all of them were converted (famous last words) to use
> > MIN_T/MAX_T instead.
> > 
> > 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>  
> 
> Eliav, your testing infrastructure needs some work, this patch breaks
> the build on this kernel tree:
> 
> In file included from ./include/linux/kernel.h:16,
>                  from ./include/linux/list.h:9,
>                  from ./include/linux/wait.h:7,
>                  from ./include/linux/wait_bit.h:8,
>                  from ./include/linux/fs.h:6,
>                  from fs/erofs/internal.h:10,
>                  from fs/erofs/zdata.h:9,
>                  from fs/erofs/zdata.c:6:
> fs/erofs/zdata.c: In function ‘z_erofs_decompress_pcluster’:
> fs/erofs/zdata.h:185:61: error: ISO C90 forbids variable length array ‘pages_onstack’ [-Werror=vla]
>   185 |         min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
>       |                                                             ^~~~

That constant seems to get (renamed and) changed to 32 in a later patch.
I'm not sure of the rational for the min() at all.
I think THREAD_SIZE is the size of the kernel stack? Or at least related to it.
The default seems to be 8k on x86-64 and 4k or 8k on i386.
So it is pretty much always going to be 96.

Linus added MIN() that can be used for array sizes.
But I'd guess this could just be changed to 32 - need to ask the erofs guys.

	David


> ./include/linux/minmax.h:49:23: note: in definition of macro ‘__cmp_once_unique’
>    49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>       |                       ^
> ./include/linux/minmax.h:164:27: note: in expansion of macro ‘__cmp_once’
>   164 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>       |                           ^~~~~~~~~~
> fs/erofs/zdata.h:185:9: note: in expansion of macro ‘min_t’
>   185 |         min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
>       |         ^~~~~
> fs/erofs/zdata.c:847:36: note: in expansion of macro ‘Z_EROFS_VMAP_ONSTACK_PAGES’
>   847 |         struct page *pages_onstack[Z_EROFS_VMAP_ONSTACK_PAGES];
>       |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> I'll drop this whole series, please do a bit more testing before sending
> out a new version.
> 
> thanks,
> 
> greg k-h
> 
RE: [PATCH v2 07/19 5.15.y] minmax: simplify and clarify min_t()/max_t() implementation
Posted by Farber, Eliav 2 months, 1 week ago
> On Mon, 6 Oct 2025 12:47:45 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> (I've had to trim the 'To' list to send this...)
>
> > On Fri, Oct 03, 2025 at 12:59:54PM +0000, Eliav Farber wrote:
> > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > >
> > > [ Upstream commit 017fa3e89187848fd056af757769c9e66ac3e93d ]
> > >
> > > This simplifies the min_t() and max_t() macros by no longer making 
> > > them work in the context of a C constant expression.
> > >
> > > That means that you can no longer use them for static initializers 
> > > or for array sizes in type definitions, but there were only a couple 
> > > of such uses, and all of them were converted (famous last words) to 
> > > use MIN_T/MAX_T instead.
> > >
> > > 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>
> >
> > Eliav, your testing infrastructure needs some work, this patch breaks 
> > the build on this kernel tree:
> >
> > In file included from ./include/linux/kernel.h:16,
> >                  from ./include/linux/list.h:9,
> >                  from ./include/linux/wait.h:7,
> >                  from ./include/linux/wait_bit.h:8,
> >                  from ./include/linux/fs.h:6,
> >                  from fs/erofs/internal.h:10,
> >                  from fs/erofs/zdata.h:9,
> >                  from fs/erofs/zdata.c:6:
> > fs/erofs/zdata.c: In function ‘z_erofs_decompress_pcluster’:
> > fs/erofs/zdata.h:185:61: error: ISO C90 forbids variable length array ‘pages_onstack’ [-Werror=vla]
> >   185 |         min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
> >       |                                                             ^~~~
>
> That constant seems to get (renamed and) changed to 32 in a later patch.
> I'm not sure of the rational for the min() at all.
> I think THREAD_SIZE is the size of the kernel stack? Or at least related to it.
> The default seems to be 8k on x86-64 and 4k or 8k on i386.
> So it is pretty much always going to be 96.
>
> Linus added MIN() that can be used for array sizes.
> But I'd guess this could just be changed to 32 - need to ask the erofs guys.

Changing the definition of Z_EROFS_VMAP_ONSTACK_PAGES to use
  MIN_T(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U)
instead of
  min_t(unsigned int, THREAD_SIZE / 8 / sizeof(struct page *), 96U).
fixes the build failure.

This aligns with the change made in upstream commit
4477b39c32fd("minmax: add a few more MIN_T/MAX_T users"):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4477b39c32fdc03363affef4b11d48391e6dc9ff

---
Regards, Eliav