Message-ID: <5597fc8f-43ed-457a-a777-4dbf52d0fbe0@lucifer.local>
Date:   Sat, 10 Jun 2023 23:35:00 +0100
From:   Lorenzo Stoakes <lstoakes@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Lu Hongfei <luhongfei@...o.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Uladzislau Rezki <urezki@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        "open list:VMALLOC" <linux-mm@...ck.org>,
        open list <linux-kernel@...r.kernel.org>,
        "opensource.kernel@...o.com" <opensource.kernel@...o.com>
Subject: Re: [PATCH] mm/vmalloc: Replace the ternary conditional operator
 with min()

On Sat, Jun 10, 2023 at 10:18:34PM +0000, David Laight wrote:
> From: Lorenzo Stoakes
> > Sent: 10 June 2023 22:07
> ...
> > > > OK, as per the pedantic test bot, you'll need to change this to:-
> > > >
> > > > num = min_t(size_t, remains, PAGE_SIZE);
> > >
> >
> > Ordinarily I wouldn't respond to this (I go into why I feel this is not
> > useful commentary below) but I am concerned Lu will take you seriously.
> >
> > > There has to be a valid reason why min/max have strong type checks.
> >
> > I really don't know what you mean by this? Yes there is a reason, I imagine
> > it's to avoid unfortunate and invalid type comparisons.
>
> Indeed, the 'unfortunate conversion' is a negative value
> being converted to a large positive one.
> That doesn't require anything like the type checking that min/max do.

Yes this is precisely what I thought it ought to be protecting again

>
> > This is not applicable here (explained below...)
> >
> > > Using min_t() all the time is just subverting them and means that
> > > bugs are more likely than if the extra tests in min() were absent.
> >
> > 'All the time' - are you just having a general whine + moan about perceived
> > kernel practices? Can you please keep it focused on the actual issues at
> > hand? I am not Linus and therefore not responsible for the entirety of the
> > kernel.
>
> I see a general problem (that Linus ought to worried about)
> is that whenever min() reports a type error the answer is
> do immediately drop in a min_t() instead of looking at the
> type of the values and fixing them to that min() doesn't complain.
> (Or fixing min() so it doesn't object to a much larger class
> of comparisons.0

Sure, but it's not really relevant here for the reasons I went
into. Probably as Andrew says elsewhere in the thread, PAGE_SIZE not being
size_t is pretty annoying.

>
> ...
> > > A 'safe' change is min(remains + 0ULL, PAGE_SIZE).
> >
> > So now we're promoting an unsigned int (and sometimes unsigned long of
> > course) to an unsigned long long (for reasons unknown) and comparing it
> > with an unsigned long? Wouldn't this trigger the sensitive type check
> > anyway?
>
> PAGE size is defined to be 'long long' - so min() will be happy.
> The compiler will just DTRT, even if 'remains' is 32bit it will
> optimise away all the implied 64-bit extension.

It's unsigned long in every arch I've seen right? And in a 32-bit arch long
long will be 64-bit, so I think you'd still have the error here.

>
> Almost all the min_t() are min_t((some unsigned type),a,b).
> If the values are known to be positive then:
> #define min_unsigned(a, b) min((a) + 0u + 0ul + 0ull, (b) + 0u + 0ul + 0ull))
> will zero extend whatever type is supplied before the comparison.
> The compiler will just discard zero extensions.
>
> > To be clear, I'd nack any such ridiculous change unless a hugely compelling
> > reason is given (you've not given any). That's horrific. And again, you've
> > not provided one single example of an _actual_ bug or situation where the
> > 'problem' you tiresomely raise would occur.
>
> The (size_t) cast isn't in itself a problem - provided you've
> checked that it is larger than the types of both sides.
> But search the kernel and you'll find places when min_t((u8),a,b)
> is used.
> This all follows the same pattern of min() gave a warning so
> so use min_t().

Yes obviously instances of inappropriate narrowing are horrible, but that
isn't happening here.

In this specific instance, for any actual architecture in reality there is
no issue.

I do absolutely agree we should address the instances where an
inappropriate type is used.

>
> ...
> > > But, in reality, min/max should always be valid when one
> > > value is a constant between 0 and MAX_INT.
> >
> > This is getting at a signed/unsigned comparison issue here afaict which is
> > not the one we're dealing with here.
>
> But it is exactly what min() is checking for and almost why min()
> exists.
> A min_unsafe() that didn't try to do any checks would be better
> than train wreck that min_t() can create.

All this ultimately sounds like a broader criticism of the min/max type
checks and not really relevant to this patch, that should be addressed at a
more general level.

>
> ...
> > Now since you kicked off this 'all the time' stuff I feel like I have been
> > given permission to make some broad comments myself...
> >
> > David, I am not one to commit-shame being a minor contributor myself buuuut
> > I see 7,610 messages from you on lore and 4 commits, all from 4 years ago
> > (please correct me if I'm wrong).
>
> I don't work for google, intel, aws (etc).
> Getting patches accepted is surprisingly hard.
>

Yes, sorry, I was probably a little too mean there (long hot day and up too
early) I didn't mean to be personal, what I was saying in blunt fashion is
the commentary needs to be proportionate to the problem and placed at the
right position, I don't feel this is.

By the way I can relate on this fully as I'm a 100% hobbyist who does this
part time (and writing a book on mm, yes I should get out more), and I know
how difficult it can be to get patches in!

> I've been writing device driver and comms protocol stack code
> for best part of 40 years.

Yes again apologies, I really didn't mean anything personal, I just found
the review a little frustrating. You are certainly more experienced than I
am! :)

Cheers, Lorenzo

>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Powered by blists - more mailing lists