Re: [PATCH] mm/vmalloc: Replace the ternary conditional operator with min()

[Lorenzo Stoakes]

On Sat, Jun 10, 2023 at 08:09:28PM +0000, David Laight wrote:
> From: Lorenzo Stoakes
> > Sent: 09 June 2023 09:49
> > On Fri, Jun 09, 2023 at 08:09:45AM +0100, Lorenzo Stoakes wrote:
> > > On Fri, Jun 09, 2023 at 02:13:09PM +0800, Lu Hongfei wrote:
> > > > It would be better to replace the traditional ternary conditional
> > > > operator with min() in zero_iter
> > > >
> > > > Signed-off-by: Lu Hongfei <luhongfei@xxxxxxxx>
> > > > ---
> > > >  mm/vmalloc.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > index 29077d61ff81..42df032e6c27
> > > > --- a/mm/vmalloc.c
> > > > +++ b/mm/vmalloc.c
> > > > @@ -3571,7 +3571,7 @@ static size_t zero_iter(struct iov_iter *iter, size_t count)
> > > >  	while (remains > 0) {
> > > >  		size_t num, copied;
> > > >
> > > > -		num = remains < PAGE_SIZE ? remains : PAGE_SIZE;
> > > > +		num = min(remains, PAGE_SIZE);
> >
> > 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. 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.

Unless this is a more specific accusation that I personally use min_t()
'all the time'? Unhelpful.

>
> The problem here is that size_t is 'unsigned int' but PAGE_SIZE
> 'unsigned long'.

The reported issue in the kernel bot report is this yes (please be more
specific!) You speak generically, but what you mean to say of course is 'in
some architectures' this is the case.

However in those same architectures, unsigned long will be equal to word
size, will it not? So the data types are in fact equivalent in all cases?
Can you propose one which is not?

If not then your whole commentary here is... just irrelevant bikesheddy
noise? Right?

I will absolutely need an example of a supported architecture where
sizeof(size_t) != sizeof(typeof(PAGE_SIZE)). I am happy to be embarrased
and to be provided one simply out of interest + learning :)

Of course even if there were such an arch, it wouldn't even matter in this
case (I go into this in next response...)

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

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.

In fact I'll nack any change along the lines of your commentary here unless
you can give a practical, compelling reason to change something rather than
broad handwaving.

I mean I'm guessing what you mean is in an unspecified architecture size_t
is unsigned int (== uint32_t) and unsigned long is uint64_t, PAGE_SIZE is
larger than 4,294,967,296 bytes and by casting first we truncate it?

This sounds very practical and I'm extremely glad you raised it.

Obviously if you can give a single example of an actual bug or issue that
could arise here (or correct me here!) I'd be interested to hear.

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

> The constant just needs forcing to 'signed int' (eg assigning
> to a temporary on that type) before the comparison (etc).

So now you're proposing a signed vs unsigned comparison? Or now is the 'one
value' not a constant but instead both need to be made signed?

Again, please explain one compelling example of any kind that gives
validity to any of your commentary here _in this specific case_ rather than
a perceived broad abuse of min_t()?

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

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

You have a reputation as somebody who loves to bikeshed and add tiresome
and trivial commentary to code reviews. I am not sure this review helps
that reputation.

Might I suggest rather than wading in with hand'a'wavin' making might I be
so bold as to say bordering on rude comments, you might do better adopting
a little courtesy and perhaps something in the way of _specific_ review
rather than fist waving at a cloud. Specificity is vital in code review.

Such courtesy is especially appreciated on drive-by reviews which I
absolutely welcome as long as the comments are sensible and presented with
courtesy, however you have failed at both here so I'm afraid I am not sure
_this specific_ commentary is quite so welcome.

Lu - please take no action based on David's comments.