[RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Adrian Bunk]

Since size_t has the same size as a long on all architectures, it's 
enough for overflow checks to check against ULONG_MAX.

This change could allow a compiler better optimization (especially in 
the n=1 case).

The practical effect seems to be positive, but quite small:

    text           data     bss      dec            hex filename
21762380        5859870 1848928 29471178        1c1b1ca vmlinux-old
21762211        5859870 1848928 29471009        1c1b121 vmlinux-patched


Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

This patch was already sent on:
- 20 Aug 2005

--- linux-2.6.13-rc6-mm1-full/include/linux/slab.h.old	2005-08-20 04:10:09.000000000 +0200
+++ linux-2.6.13-rc6-mm1-full/include/linux/slab.h	2005-08-20 04:11:04.000000000 +0200
@@ -113,7 +113,7 @@
  */
 static inline void *kcalloc(size_t n, size_t size, unsigned int __nocast flags)
 {
-	if (n != 0 && size > INT_MAX / n)
+	if (n != 0 && size > ULONG_MAX / n)
 		return NULL;
 	return kzalloc(n * size, flags);
 }

Re: [RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Adrian Bunk]

On Sun, Aug 21, 2005 at 11:12:06PM +0300, Pekka Enberg wrote:
> On Sun, Aug 21, 2005 at 10:47:13PM +0300, Pekka Enberg wrote:
> > > You'll probably get even better code if you change the above to:
> > >
> > >     if (size != 0 && n > ULONG_MAX / size)
> > >
> > > Reason being that size is virtually always a constant so the compiler
> > > can evaluate the division at compile-time.
> 
> On 8/21/05, Adrian Bunk <bunk@stusta.de> wrote:
> > I doubt this would make any difference.
> > 
> > And besides, except in some rare cases, the second argument is a
> > sizeof(foo) whose size is already known at compile time.
> 
> Yes, that's my point. The second argument (size) is virtually always
> sizeof() whereas the first one (n) is sometimes a variable. GCC
> currently does not optimize away the division when n is not a
> constant.
> 
> Looking at 2.6.13-rc6-mm1, we have roughly 15 callers with the first
> parameter being a variable. The compiler would be able to get rid of
> one comparison and division instruction for each of these so looks
> like we could shave off some more bytes...

With gcc 4.0.1:

    text           data     bss     dec             hex filename
25675334        5851630 1819976 33346940        1fcd57c vmlinux-my-patch
25675366        5851630 1819976 33346972        1fcd59c vmlinux-your-patch

INT_MAX -> ULONG_MAX is correct, even though it doesn't seem to make a 
difference with today's gcc.

Trying to change the code in a way that gcc will produce better code 
doesn't seem to be worth it (except in extreme hot paths).

>                             Pekka

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Pekka Enberg]

On Sun, Aug 21, 2005 at 10:47:13PM +0300, Pekka Enberg wrote:
> > You'll probably get even better code if you change the above to:
> >
> >     if (size != 0 && n > ULONG_MAX / size)
> >
> > Reason being that size is virtually always a constant so the compiler
> > can evaluate the division at compile-time.

On 8/21/05, Adrian Bunk <bunk@stusta.de> wrote:
> I doubt this would make any difference.
> 
> And besides, except in some rare cases, the second argument is a
> sizeof(foo) whose size is already known at compile time.

Yes, that's my point. The second argument (size) is virtually always
sizeof() whereas the first one (n) is sometimes a variable. GCC
currently does not optimize away the division when n is not a
constant.

Looking at 2.6.13-rc6-mm1, we have roughly 15 callers with the first
parameter being a variable. The compiler would be able to get rid of
one comparison and division instruction for each of these so looks
like we could shave off some more bytes...

                            Pekka

Re: [RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Adrian Bunk]

On Sun, Aug 21, 2005 at 10:47:13PM +0300, Pekka Enberg wrote:
> On 8/20/05, Adrian Bunk <bunk@stusta.de> wrote:
> > This change could (at least in theory) allow a compiler better
> > optimization (especially in the n=1 case).
> > 
> > The practical effect seems to be nearly zero:
> >     text           data     bss      dec            hex filename
> > 25617207        5850138 1827016 33294361        1fc0819 vmlinux-old
> > 25617191        5850138 1827016 33294345        1fc0809 vmlinux-patched
> > 
> > Is there any reason against this patch?
> 
> Looks ok to me.
> 
> On 8/20/05, Adrian Bunk <bunk@stusta.de> wrote:
> >  static inline void *kcalloc(size_t n, size_t size, unsigned int __nocast flags)
> >  {
> > -       if (n != 0 && size > INT_MAX / n)
> > +       if (n != 0 && size > ULONG_MAX / n)
> 
> You'll probably get even better code if you change the above to:
> 
>     if (size != 0 && n > ULONG_MAX / size)
> 
> Reason being that size is virtually always a constant so the compiler
> can evaluate the division at compile-time.

I doubt this would make any difference.

And besides, except in some rare cases, the second argument is a 
sizeof(foo) whose size is already known at compile time.

>                                   Pekka

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Pekka Enberg]

On 8/20/05, Adrian Bunk <bunk@stusta.de> wrote:
> This change could (at least in theory) allow a compiler better
> optimization (especially in the n=1 case).
> 
> The practical effect seems to be nearly zero:
>     text           data     bss      dec            hex filename
> 25617207        5850138 1827016 33294361        1fc0819 vmlinux-old
> 25617191        5850138 1827016 33294345        1fc0809 vmlinux-patched
> 
> Is there any reason against this patch?

Looks ok to me.

On 8/20/05, Adrian Bunk <bunk@stusta.de> wrote:
>  static inline void *kcalloc(size_t n, size_t size, unsigned int __nocast flags)
>  {
> -       if (n != 0 && size > INT_MAX / n)
> +       if (n != 0 && size > ULONG_MAX / n)

You'll probably get even better code if you change the above to:

    if (size != 0 && n > ULONG_MAX / size)

Reason being that size is virtually always a constant so the compiler
can evaluate the division at compile-time.

                                  Pekka

[RFC: -mm patch] kcalloc(): INT_MAX -> ULONG_MAX

[Adrian Bunk]

This change could (at least in theory) allow a compiler better 
optimization (especially in the n=1 case).

The practical effect seems to be nearly zero:
    text           data     bss      dec            hex filename
25617207        5850138 1827016 33294361        1fc0819 vmlinux-old
25617191        5850138 1827016 33294345        1fc0809 vmlinux-patched

Is there any reason against this patch?


Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.13-rc6-mm1-full/include/linux/slab.h.old	2005-08-20 04:10:09.000000000 +0200
+++ linux-2.6.13-rc6-mm1-full/include/linux/slab.h	2005-08-20 04:11:04.000000000 +0200
@@ -113,7 +113,7 @@
  */
 static inline void *kcalloc(size_t n, size_t size, unsigned int __nocast flags)
 {
-	if (n != 0 && size > INT_MAX / n)
+	if (n != 0 && size > ULONG_MAX / n)
 		return NULL;
 	return kzalloc(n * size, flags);
 }