Message-Id: <e946e002-8ca8-4a09-a800-d117c89b39d3@app.fastmail.com>
Date: Tue, 30 Jul 2024 00:25:24 +0200
From: "Arnd Bergmann" <arnd@...nel.org>
To: "Linus Torvalds" <torvalds@...uxfoundation.org>,
 "David Laight" <David.Laight@...lab.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "Jens Axboe" <axboe@...nel.dk>, "Matthew Wilcox" <willy@...radead.org>,
 "Christoph Hellwig" <hch@...radead.org>,
 "Andrew Morton" <akpm@...ux-foundation.org>,
 "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
 "Dan Carpenter" <dan.carpenter@...aro.org>,
 "Jason A . Donenfeld" <Jason@...c4.com>,
 "pedro.falcato@...il.com" <pedro.falcato@...il.com>,
 "Mateusz Guzik" <mjguzik@...il.com>,
 "linux-mm@...ck.org" <linux-mm@...ck.org>,
 "Lorenzo Stoakes" <lorenzo.stoakes@...cle.com>
Subject: Re: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

On Sun, Jul 28, 2024, at 22:13, Linus Torvalds wrote:
> On Sun, 28 Jul 2024 at 13:10, David Laight <David.Laight@...lab.com> wrote:
>>
>> I think they just need to be MIN_CONST() (without the casts).
>
> I'll just convert the existing cases of min_t/max_t to MIN_T/MAX_T,
> which I already added for other reasons anyway.
>
> That makes min_t/max_t not have to care about the nasty special cases
> (really just array sizes in these cases, and they all wanted MAX_T).

I had prototyped something similar end of last week but didn't manage
to get my version out to you before the weekend. Comparing mine with
what you ended up committing:

- You found exactly the same array index uses I found in
  randconfig testing, so I'm not aware of anything missing
  there.

- My macros use __builtin_choose_expr() instead of ?: to
  ensure that the arguments are constant, this produces a
  relatively clear compiler warning when they are not.
  Without that, I would expect random drivers to start
  using MIN()/MAX() in places where it's not safe.

- I went with the belts-and-suspenders version of MIN()/MAX()
  that works when comparing a negative constant against
  an unsigned one. This requires expanding each argument
  four or five times instead of two, so you might still
  want the simpler version (like MIN_T/MAX_T):

--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -295,12 +271,18 @@ static inline bool in_range32(u32 val, u32 start, u32 len)
        do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
 
 /*
- * Use these carefully: no type checking, and uses the arguments
- * multiple times. Use for obvious constants only.
+ * These only work on constant values but return a constant value that
+ * can be used as an array size
  */
-#define MIN(a,b) __cmp(min,a,b)
-#define MAX(a,b) __cmp(max,a,b)
-#define MIN_T(type,a,b) __cmp(min,(type)(a),(type)(b))
-#define MAX_T(type,a,b) __cmp(max,(type)(a),(type)(b))
+#define MIN(x, y) \
+   __builtin_choose_expr(((x) < 0 && (y) > 0), (x), \
+   __builtin_choose_expr((((y) < 0 && (x) > 0) || (y) < (x)), (y), (x)))
+
+#define MAX(x, y) \
+   __builtin_choose_expr(((x) > 0 && (y) < 0), (x), \
+   __builtin_choose_expr((((y) > 0 && (x) < 0) || (y) > (x)), (y), (x)))
+
+#define MIN_T(type,a,b) (type)__builtin_choose_expr((type)(a) < (type)(b), (a), (b))
+#define MAX_T(type,a,b) (type)__builtin_choose_expr((type)(a) > (type)(b), (a), (b))
 
 #endif /* _LINUX_MINMAX_H */

- The change above requires changing a number of files that were
  previously using their own MIN()/MAX() macros over to using
  min()/max(), as they are passing non-constant values in:

 drivers/gpu/drm/amd/display/dc/core/dc_stream.c             | 12 ++++--------
 .../drm/amd/display/dc/dio/dcn20/dcn20_link_encoder.c       |  9 +--------
 .../drm/amd/display/dc/dio/dcn31/dcn31_dio_link_encoder.c   |  8 ++------
 .../drm/amd/display/dc/dio/dcn32/dcn32_dio_link_encoder.c   |  6 +-----
 .../drm/amd/display/dc/dio/dcn321/dcn321_dio_link_encoder.c |  4 ----
 .../drm/amd/display/dc/dio/dcn401/dcn401_dio_link_encoder.c |  8 --------
 .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c                | 13 +++----------
 .../drm/amd/display/dc/dsc/dc_dsc.c                         |  9 +--------
 .../drm/amd/display/dc/link/protocols/link_dp_capability.c  | 13 +++----------
 drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c         | 11 ++++-------
 drivers/gpu/drm/radeon/evergreen_cs.c                       |  9 ++-------
 drivers/platform/x86/sony-laptop.c                          |  4 ++--
 kernel/trace/preemptirq_delay_test.c                        |  2 +-
 lib/decompress_unlzma.c                                     |  7 ++-----
 14 files changed, 26 insertions(+), 89 deletions(-)

  Changing these is probably a good idea regardless.

- I also tried simplifying __types_ok()  further, which as
  you already  mentioned doesn't easily work with pointer
  arguments. Again we could work around this with a separate
  min_ptr()/max_ptr() helper. I only found 11 files that
  actually compare pointers (on x86/arm/arm64 randconfig):  

 arch/arm64/kvm/hyp/nvhe/page_alloc.c  | 2 +-
 crypto/skcipher.c                     | 2 +-
 drivers/gpu/drm/drm_modes.c           | 2 +-
 drivers/infiniband/hw/hfi1/pio_copy.c | 4 ++--
 drivers/irqchip/irq-bcm7120-l2.c      | 2 +-
 drivers/spi/spi-cs42l43.c             | 8 ++++----
 fs/ntfs3/lznt.c                       | 2 +-
 lib/lzo/lzo1x_compress.c              | 2 +-
 mm/kmemleak.c                         | 4 ++--
 mm/percpu.c                           | 2 +-
 net/ceph/osdmap.c                     | 6 +++---
 11 files changed, 25 insertions(+), 18 deletions(-)

 The simpler __types_ok() needs more testing across all
 compiler versions, so that wouldn't be for 6.11 anyway.
 I can send the min_ptr()/max_ptr() stuff anyway if
 you think that's a good idea.  

       Arnd

Powered by blists - more mailing lists