Message-ID: <20251121091504.41ada607@pumpkin>
Date: Fri, 21 Nov 2025 09:15:04 +0000
From: David Laight <david.laight.linux@...il.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: "David Hildenbrand (Red Hat)" <david@...nel.org>,
 linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, Axel
 Rasmussen <axelrasmussen@...gle.com>, Christoph Lameter <cl@...two.org>,
 Dennis Zhou <dennis@...nel.org>, Johannes Weiner <hannes@...xchg.org>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>, Mike Rapoport
 <rppt@...nel.org>, Tejun Heo <tj@...nel.org>, Yuanchu Xie
 <yuanchu@...gle.com>
Subject: Re: [PATCH 39/44] mm: use min() instead of min_t()

On Thu, 20 Nov 2025 23:45:22 +0000
Eric Biggers <ebiggers@...nel.org> wrote:

> On Thu, Nov 20, 2025 at 09:59:46AM +0000, David Laight wrote:
> > On Thu, 20 Nov 2025 10:20:41 +0100
> > "David Hildenbrand (Red Hat)" <david@...nel.org> wrote:
> >   
> > > On 11/19/25 23:41, david.laight.linux@...il.com wrote:  
> > > > From: David Laight <david.laight.linux@...il.com>
> > > > 
> > > > min_t(unsigned int, a, b) casts an 'unsigned long' to 'unsigned int'.
> > > > Use min(a, b) instead as it promotes any 'unsigned int' to 'unsigned long'
> > > > and so cannot discard significant bits.    
> > > 
> > > I thought using min() was frowned upon and we were supposed to use 
> > > min_t() instead to make it clear which type we want to use.  
> > 
> > I'm not sure that was ever true.
> > min_t() is just an accident waiting to happen.
> > (and I found a few of them, the worst are in sched/fair.c)
> > 
> > Most of the min_t() are there because of the rather overzealous type
> > check that used to be in min().
> > But even then it would really be better to explicitly cast one of the
> > parameters to min(), so min_t(T, a, b) => min(a, (T)b).
> > Then it becomes rather more obvious that min_t(u8, x->m_u8, expr)
> > is going mask off the high bits of 'expr'.
> >   
> > > Do I misremember or have things changed?
> > > 
> > > Wasn't there a checkpatch warning that states exactly that?  
> > 
> > There is one that suggests min_t() - it ought to be nuked.
> > The real fix is to backtrack the types so there isn't an error.
> > min_t() ought to be a 'last resort' and a single cast is better.
> > 
> > With the relaxed checks in min() most of the min_t() can just
> > be replaced by min(), even this is ok:
> > 	int len = fun();
> > 	if (len < 0)
> > 		return;
> > 	count = min(len, sizeof(T));
> > 
> > I did look at the history of min() and min_t().
> > IIRC some of the networking code had a real function min() with
> > 'unsigned int' arguments.
> > This was moved to a common header, changed to a #define and had
> > a type added - so min(T, a, b).
> > Pretty much immediately that was renamed min_t() and min() added
> > that accepted any type - but checked the types of 'a' and 'b'
> > exactly matched.
> > Code was then changed (over the years) to use min(), but in many
> > cases the types didn't quite match - so min_t() was used a lot.
> > 
> > I keep spotting new commits that pass too small a type to min_t().
> > So this is the start of a '5 year' campaign to nuke min_t() (et al).  
> 
> Yes, checkpatch suggests min_t() or max_t() if you cast an argument to
> min() or max().  Grep for "typecasts on min/max could be min_t/max_t" in
> scripts/checkpatch.pl.

IMHO that is a really bad suggestion (and always has been).
In reality min(a, (T)b) is less likely to be buggy than min_t(T, a, b).
Someone will notice that (u16)long_var is likely to be buggy but min_t()
is expected to 'do something magic'.

There are a log of examples of 'T_var = min_t(T, T_var, b)' which really
needed (typeof (b))T_var rather than (T)b
and T_var = min_t(T, a, b) which just doesn't need a cast at all.


> 
> And historically you could not pass different types to min() and max(),
> which is why people use min_t() and max_t().  It looks like you fixed
> that a couple years ago in
> https://lore.kernel.org/all/b97faef60ad24922b530241c5d7c933c@AcuMS.aculab.com/,
> which is great!

I wrote that, and then Linus redid it to avoid some very long lines
from nested expansion (with some tree-wide patches that only he could do).

>  It just takes some time for the whole community to get
> the message.  Also, it seems that checkpatch is in need of an update.
> 
> Doing these conversions looks good to me, but unfortunately this is
> probably the type of thing that shouldn't be a single kernel-wide patch
> series.  They should be sent out per-subsystem.

In effect it is a list of separate patches, one per subsystem.
They just have a common 0/n wrapper.
I wanted to link them together, I guess I could have put a bit more
text in the common commit message I pasted into all the commits.

I didn't post the change to minmax.h (apart from a summary in 0/44)
because I hadn't even tried to build a 32bit kernel nevery mind
an allmodconfig or allyesconfig one.

I spent all yesterday trying to build allyesconfig...

	David

> 
> I suggest also putting a sentence in the commit message that mentions
> that min() and max() have been updated to accept arguments with
> different types.  (Seeing as historically that wasn't true.)  I suggest
> also being extra clear about when each change is a cleanup vs a fix. 
> 
> - Eric

Powered by blists - more mailing lists