LinuxLists.cc - [PATCH] mm/vmalloc: Replace the ternary conditional operator with min()

2023-06-09 06:20:43

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

It would be better to replace the traditional ternary conditional
operator with min() in zero_iter

Signed-off-by: Lu Hongfei <[email protected]>
---
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);
copied = copy_page_to_iter_nofault(ZERO_PAGE(0), 0, num, iter);
remains -= copied;

--
2.39.0

2023-06-09 07:15:33

by Lorenzo Stoakes

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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);
> copied = copy_page_to_iter_nofault(ZERO_PAGE(0), 0, num, iter);
> remains -= copied;
>
> --
> 2.39.0
>

Looks good to me, thanks,

Reviewed-by: Lorenzo Stoakes <[email protected]>

2023-06-09 08:53:12

by kernel test robot

[permalink] [raw]

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

Hi Lu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url: https://github.com/intel-lab-lkp/linux/commits/Lu-Hongfei/mm-vmalloc-Replace-the-ternary-conditional-operator-with-min/20230609-141417
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20230609061309.42453-1-luhongfei%40vivo.com
patch subject: [PATCH] mm/vmalloc: Replace the ternary conditional operator with min()
config: hexagon-randconfig-r016-20230608 (https://download.01.org/0day-ci/archive/20230609/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add akpm-mm https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git
git fetch akpm-mm mm-everything
git checkout akpm-mm/mm-everything
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from mm/vmalloc.c:14:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from mm/vmalloc.c:14:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from mm/vmalloc.c:14:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> mm/vmalloc.c:3574:9: warning: comparison of distinct pointer types ('typeof (remains) *' (aka 'unsigned int *') and 'typeof ((1UL << 12)) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
num = min(remains, PAGE_SIZE);
^~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:67:19: note: expanded from macro 'min'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
7 warnings generated.

vim +3574 mm/vmalloc.c

3561
3562 /*
3563 * Atomically zero bytes in the iterator.
3564 *
3565 * Returns the number of zeroed bytes.
3566 */
3567 static size_t zero_iter(struct iov_iter *iter, size_t count)
3568 {
3569 size_t remains = count;
3570
3571 while (remains > 0) {
3572 size_t num, copied;
3573
> 3574 num = min(remains, PAGE_SIZE);
3575 copied = copy_page_to_iter_nofault(ZERO_PAGE(0), 0, num, iter);
3576 remains -= copied;
3577
3578 if (copied < num)
3579 break;
3580 }
3581
3582 return count - remains;
3583 }
3584

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-06-09 08:54:42

by Lorenzo Stoakes

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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);

> > copied = copy_page_to_iter_nofault(ZERO_PAGE(0), 0, num, iter);
> > remains -= copied;
> >
> > --
> > 2.39.0
> >
>
> Looks good to me, thanks,
>
> Reviewed-by: Lorenzo Stoakes <[email protected]>

Please spin up a v2 with this change and then you can take my Reviewed-by tag :)

Cheers, Lorenzo

2023-06-09 09:31:20

by kernel test robot

[permalink] [raw]

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

2023-06-09 10:09:00

by kernel test robot

[permalink] [raw]

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

2023-06-10 20:22:15

by David Laight

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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);

There has to be a valid reason why min/max have strong type checks.
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.

The problem here is that size_t is 'unsigned int' but PAGE_SIZE
'unsigned long'.
A 'safe' change is min(remains + 0ULL, PAGE_SIZE).

But, in reality, min/max should always be valid when one
value is a constant between 0 and MAX_INT.
The constant just needs forcing to 'signed int' (eg assigning
to a temporary on that type) before the comparison (etc).

David

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

2023-06-10 22:06:04

by Lorenzo Stoakes

[permalink] [raw]

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

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 <[email protected]>
> > > > ---
> > > > 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.

2023-06-10 22:17:57

by Andrew Morton

[permalink] [raw]

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

On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <[email protected]> wrote:

> > > OK, as per the pedantic test bot, you'll need to change this to:-
> > >
> > > num = min_t(size_t, remains, PAGE_SIZE);

PAGE_SIZE is a nuisance. It _usually_ creates the need for a
cast:

hp2:/usr/src/linux-6.4-rc4> grep -r "min(.*PAGE_SIZE" . | wc -l
117
hp2:/usr/src/linux-6.4-rc4> grep -r "min_t(.*PAGE_SIZE" . | wc -l
279

Perhaps it should always have been size_t.

I suppose we could do

#define PAGE_SIZE_T (size_t)PAGE_SIZE

And use that where needed. Mainly because I like the name ;)

2023-06-10 22:35:34

by Lorenzo Stoakes

[permalink] [raw]

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

On Sat, Jun 10, 2023 at 03:08:09PM -0700, Andrew Morton wrote:
> On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <[email protected]> wrote:
>
> > > > OK, as per the pedantic test bot, you'll need to change this to:-
> > > >
> > > > num = min_t(size_t, remains, PAGE_SIZE);
>
> PAGE_SIZE is a nuisance. It _usually_ creates the need for a
> cast:
>
> hp2:/usr/src/linux-6.4-rc4> grep -r "min(.*PAGE_SIZE" . | wc -l
> 117
> hp2:/usr/src/linux-6.4-rc4> grep -r "min_t(.*PAGE_SIZE" . | wc -l
> 279
>
> Perhaps it should always have been size_t.
>

I mean I absolutely agree this not being size_t is a bit silly although I'm
pretty convinced sizeof(size_t) == sizeof(unsigned long) in all kernel
arches (correct me if I'm wrong).

So I'd not be against this at all :)

> I suppose we could do
>
> #define PAGE_SIZE_T (size_t)PAGE_SIZE
>
> And use that where needed. Mainly because I like the name ;)

I'm not sure I love that name :P but it is kind of cute I guess!

2023-06-10 22:37:24

by David Laight

[permalink] [raw]

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

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.

> 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

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

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

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

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

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

David

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

2023-06-10 23:06:53

by David Laight

[permalink] [raw]

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

From: Andrew Morton <[email protected]>
> Sent: 10 June 2023 23:08
>
> On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <[email protected]> wrote:
>
> > > > OK, as per the pedantic test bot, you'll need to change this to:-
> > > >
> > > > num = min_t(size_t, remains, PAGE_SIZE);
>
> PAGE_SIZE is a nuisance. It _usually_ creates the need for a
> cast:
>
> hp2:/usr/src/linux-6.4-rc4> grep -r "min(.*PAGE_SIZE" . | wc -l
> 117
> hp2:/usr/src/linux-6.4-rc4> grep -r "min_t(.*PAGE_SIZE" . | wc -l
> 279
>
> Perhaps it should always have been size_t.
>
> I suppose we could do
>
> #define PAGE_SIZE_T (size_t)PAGE_SIZE
>
> And use that where needed. Mainly because I like the name ;)

Or someone take my patches to relax the checks min() does a bit.
I think I last posted them in January.
Basically:
- unsigned v unsigned is always ok.
- signed v signed is always ok.
- unsigned v signed is ok provided one value is in [0..INT_MAX].
this can be allowed for compile-time constants.

The usual 'error case' is unsigned v signed when the values
are known (by the person writing the code) to be non-negative.
Doing '(x) + 0u + 0ul + 0ull' zero extends the value without
ever masking it or 'accidentally' converting a pointer.

David

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

2023-06-11 00:35:09

by Lorenzo Stoakes

[permalink] [raw]

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