Message-ID: <528997fd-5dcb-4332-845b-18828931417d@kernel.org>
Date: Fri, 21 Nov 2025 09:27:42 +0100
From: "David Hildenbrand (Red Hat)" <david@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>,
 David Laight <david.laight.linux@...il.com>
Cc: 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 11/21/25 00:45, Eric Biggers 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.

Right, that's the one I recalled.

> 
> 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!  It just takes some time for the whole community to get
> the message.  Also, it seems that checkpatch is in need of an update.

Exactly.

And whenever it comes to such things, I wonder if we want to clearly 
spell them out somewhere (codying-style): especially, when to use 
min/max and when to use min_t/max_t.

coding-style currently mentions:

"There are also min() and max() macros that do strict type checking ..." 
is that also outdated or am I just confused at this point?

> 
> 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.

Agreed!

In particular as there is no need to rush and individual subsystems can 
just pick it up separately.

> 
> 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.

+1

-- 
Cheers

David

Powered by blists - more mailing lists