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

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

[Lu Hongfei]

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

Signed-off-by: Lu Hongfei <luhongfei@vivo.com>
---
 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

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

[Lorenzo Stoakes]

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@vivo.com>
> ---
>  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 <lstoakes@gmail.com>

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

[kernel test robot]

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/202306091647.GzAdThKz-lkp@intel.com/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/20230609061309.42453-1-luhongfei@vivo.com
        # 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306091647.GzAdThKz-lkp@intel.com/

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

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

[Lorenzo Stoakes]

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@vivo.com>
> > ---
> >  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 <lstoakes@gmail.com>

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

Cheers, Lorenzo

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

[kernel test robot]

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: arc-randconfig-s032-20230609 (https://download.01.org/0day-ci/archive/20230609/202306091704.nUl4tysX-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.3.0
reproduce:
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/a85ea29b355934c588aeb97faf6846b76d512a6d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lu-Hongfei/mm-vmalloc-Replace-the-ternary-conditional-operator-with-min/20230609-141417
        git checkout a85ea29b355934c588aeb97faf6846b76d512a6d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arc 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306091704.nUl4tysX-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> mm/vmalloc.c:3574:23: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> mm/vmalloc.c:3574:23: sparse:    unsigned int *
>> mm/vmalloc.c:3574:23: sparse:    unsigned long *
   mm/vmalloc.c:4291:13: sparse: sparse: context imbalance in 's_start' - wrong count at exit
   mm/vmalloc.c:4306:13: sparse: sparse: context imbalance in 's_stop' - wrong count at exit

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

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

[kernel test robot]

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: i386-randconfig-i011-20230608 (https://download.01.org/0day-ci/archive/20230609/202306091701.KHIG4Osf-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
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/20230609061309.42453-1-luhongfei@vivo.com
        # 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=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306091701.KHIG4Osf-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> 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)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning 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

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

[David Laight]

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@vivo.com>
> > > ---
> > >  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)

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@vivo.com>
> > > > ---
> > > >  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.

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

[Andrew Morton]

On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <lstoakes@gmail.com> 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 ;)

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

[David Laight]

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)

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

[Lorenzo Stoakes]

On Sat, Jun 10, 2023 at 03:08:09PM -0700, Andrew Morton wrote:
> On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <lstoakes@gmail.com> 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!

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

[David Laight]

From: Andrew Morton <akpm@linux-foundation.org>
> Sent: 10 June 2023 23:08
> 
> On Sat, 10 Jun 2023 22:06:35 +0100 Lorenzo Stoakes <lstoakes@gmail.com> 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)

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

[Lorenzo Stoakes]

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