[PATCH] Uninline kcalloc()

[PATCH] Uninline kcalloc()

[Alexey Dobriyan]

This saves some bytes on usual config here and allows inserting integer
overflow warning without pissing off printk-haters crowd later on.

   text    data     bss     dec     hex filename
2662791  195347  159744 3017882  2e0c9a vmlinux		-O2 before
2662535  195347  159744 3017626  2e0b9a vmlinux		-O2 after
-------------------------------
			   -256

2439726  195347  159744 2794817  2aa541 vmlinux		-Os before
2439598  195347  159744 2794689  2aa4c1 vmlinux		-Os after
-------------------------------
			   -128

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/linux/slab.h |   58 ----------------------------------------
 mm/util.c            |   59 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 57 deletions(-)

--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -120,63 +120,7 @@ size_t ksize(const void *);
 #include <linux/slab_def.h>
 #endif
 
-/**
- * kcalloc - allocate memory for an array. The memory is set to zero.
- * @n: number of elements.
- * @size: element size.
- * @flags: the type of memory to allocate.
- *
- * The @flags argument may be one of:
- *
- * %GFP_USER - Allocate memory on behalf of user.  May sleep.
- *
- * %GFP_KERNEL - Allocate normal kernel ram.  May sleep.
- *
- * %GFP_ATOMIC - Allocation will not sleep.  May use emergency pools.
- *   For example, use this inside interrupt handlers.
- *
- * %GFP_HIGHUSER - Allocate pages from high memory.
- *
- * %GFP_NOIO - Do not do any I/O at all while trying to get memory.
- *
- * %GFP_NOFS - Do not make any fs calls while trying to get memory.
- *
- * %GFP_NOWAIT - Allocation will not sleep.
- *
- * %GFP_THISNODE - Allocate node-local memory only.
- *
- * %GFP_DMA - Allocation suitable for DMA.
- *   Should only be used for kmalloc() caches. Otherwise, use a
- *   slab created with SLAB_DMA.
- *
- * Also it is possible to set different flags by OR'ing
- * in one or more of the following additional @flags:
- *
- * %__GFP_COLD - Request cache-cold pages instead of
- *   trying to return cache-warm pages.
- *
- * %__GFP_HIGH - This allocation has high priority and may use emergency pools.
- *
- * %__GFP_NOFAIL - Indicate that this allocation is in no way allowed to fail
- *   (think twice before using).
- *
- * %__GFP_NORETRY - If memory is not immediately available,
- *   then give up at once.
- *
- * %__GFP_NOWARN - If allocation fails, don't issue any warnings.
- *
- * %__GFP_REPEAT - If allocation fails initially, try once more before failing.
- *
- * There are other flags available as well, but these are not intended
- * for general use, and so are not documented here. For a full list of
- * potential flags, always refer to linux/gfp.h.
- */
-static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
-{
-	if (n != 0 && size > ULONG_MAX / n)
-		return NULL;
-	return __kmalloc(n * size, flags | __GFP_ZERO);
-}
+void *kcalloc(size_t n, size_t size, gfp_t flags);
 
 #if !defined(CONFIG_NUMA) && !defined(CONFIG_SLOB)
 /**
--- a/mm/util.c
+++ b/mm/util.c
@@ -5,6 +5,65 @@
 #include <asm/uaccess.h>
 
 /**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate.
+ *
+ * The @flags argument may be one of:
+ *
+ * %GFP_USER - Allocate memory on behalf of user.  May sleep.
+ *
+ * %GFP_KERNEL - Allocate normal kernel ram.  May sleep.
+ *
+ * %GFP_ATOMIC - Allocation will not sleep.  May use emergency pools.
+ *   For example, use this inside interrupt handlers.
+ *
+ * %GFP_HIGHUSER - Allocate pages from high memory.
+ *
+ * %GFP_NOIO - Do not do any I/O at all while trying to get memory.
+ *
+ * %GFP_NOFS - Do not make any fs calls while trying to get memory.
+ *
+ * %GFP_NOWAIT - Allocation will not sleep.
+ *
+ * %GFP_THISNODE - Allocate node-local memory only.
+ *
+ * %GFP_DMA - Allocation suitable for DMA.
+ *   Should only be used for kmalloc() caches. Otherwise, use a
+ *   slab created with SLAB_DMA.
+ *
+ * Also it is possible to set different flags by OR'ing
+ * in one or more of the following additional @flags:
+ *
+ * %__GFP_COLD - Request cache-cold pages instead of
+ *   trying to return cache-warm pages.
+ *
+ * %__GFP_HIGH - This allocation has high priority and may use emergency pools.
+ *
+ * %__GFP_NOFAIL - Indicate that this allocation is in no way allowed to fail
+ *   (think twice before using).
+ *
+ * %__GFP_NORETRY - If memory is not immediately available,
+ *   then give up at once.
+ *
+ * %__GFP_NOWARN - If allocation fails, don't issue any warnings.
+ *
+ * %__GFP_REPEAT - If allocation fails initially, try once more before failing.
+ *
+ * There are other flags available as well, but these are not intended
+ * for general use, and so are not documented here. For a full list of
+ * potential flags, always refer to linux/gfp.h.
+ */
+void *kcalloc(size_t n, size_t size, gfp_t flags)
+{
+	if (n != 0 && size > ULONG_MAX / n)
+		return NULL;
+	return __kmalloc(n * size, flags | __GFP_ZERO);
+}
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kstrdup - allocate space for and copy an existing string
  * @s: the string to duplicate
  * @gfp: the GFP mask used in the kmalloc() call when allocating memory

Re: [PATCH] Uninline kcalloc()

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:

> -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
> -{
> -	if (n != 0 && size > ULONG_MAX / n)
> -		return NULL;
> -	return __kmalloc(n * size, flags | __GFP_ZERO);
> -}
> +void *kcalloc(size_t n, size_t size, gfp_t flags);

NAK.

This busticates some pretty subtle code in mm/slab.c that uses
uses __builtin_return_address() for debugging - if you do this, then
the "calling function" gets listed as "kcalloc()" rather than the much more
useful "function that called kcalloc()" (which is what you care about).

(I remember going around and around multiple times getting those stupid
inlines set up right, so that feature actually did something useful, otherwise
kcalloc and kzalloc didn't report where they were called from).

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Uninline kcalloc()

[Jan Engelhardt]

On Sep 24 2007 01:35, Valdis.Kletnieks@vt.edu wrote:
>On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:
>
>> -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
>> -{
>> -	if (n != 0 && size > ULONG_MAX / n)
>> -		return NULL;
>> -	return __kmalloc(n * size, flags | __GFP_ZERO);
>> -}
>> +void *kcalloc(size_t n, size_t size, gfp_t flags);
>
>NAK.
>
>This busticates some pretty subtle code in mm/slab.c that uses
>uses __builtin_return_address() for debugging - if you do this, then
>the "calling function" gets listed as "kcalloc()" rather than the much more
>useful "function that called kcalloc()" (which is what you care about).
>
>(I remember going around and around multiple times getting those stupid
>inlines set up right, so that feature actually did something useful, otherwise
>kcalloc and kzalloc didn't report where they were called from).

Since 'inline' is only a hint , should not it be __always_inline,
so that __builtin_return_address() always works?

Re: [PATCH] Uninline kcalloc()

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Mon, 24 Sep 2007 08:22:31 +0200, Jan Engelhardt said:
> 
> On Sep 24 2007 01:35, Valdis.Kletnieks@vt.edu wrote:
> >On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:
> >
> >> -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
> >> -{
> >> -	if (n != 0 && size > ULONG_MAX / n)
> >> -		return NULL;
> >> -	return __kmalloc(n * size, flags | __GFP_ZERO);
> >> -}
> >> +void *kcalloc(size_t n, size_t size, gfp_t flags);
> >
> >NAK.
> >
> >This busticates some pretty subtle code in mm/slab.c that uses
> >uses __builtin_return_address() for debugging - if you do this, then
> >the "calling function" gets listed as "kcalloc()" rather than the much more
> >useful "function that called kcalloc()" (which is what you care about).
> >
> >(I remember going around and around multiple times getting those stupid
> >inlines set up right, so that feature actually did something useful, otherwise
> >kcalloc and kzalloc didn't report where they were called from).
> 
> Since 'inline' is only a hint , should not it be __always_inline,
> so that __builtin_return_address() always works?

At the time I was trying to fix that, saying "inline" actually forced it
to be so.  Probably need to go re-check that, since there's been about
forty-leven different "tweak the semantics of inline" since then....

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Uninline kcalloc()

[Alexey Dobriyan]

On 9/24/07, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:
>
> > -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
> > -{
> > -	if (n != 0 && size > ULONG_MAX / n)
> > -		return NULL;
> > -	return __kmalloc(n * size, flags | __GFP_ZERO);
> > -}
> > +void *kcalloc(size_t n, size_t size, gfp_t flags);
>
> NAK.
>
> This busticates some pretty subtle code in mm/slab.c that uses
> uses __builtin_return_address() for debugging

Interesting. Here is output from kernel with patch applied and leak
plugged into proc_dointvec() (I checked twice):

        $ grep kcalloc /proc/slab_allocators
        $ grep proc_dointvec /proc/slab_allocators
        size-64: 19 proc_dointvec+0x48/0xa0

Re: [PATCH] Uninline kcalloc()

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Mon, 24 Sep 2007 11:44:35 +0400, Alexey Dobriyan said:
> On 9/24/07, Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> > On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:
> >
> > > -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
> > > -{
> > > -	if (n != 0 && size > ULONG_MAX / n)
> > > -		return NULL;
> > > -	return __kmalloc(n * size, flags | __GFP_ZERO);
> > > -}
> > > +void *kcalloc(size_t n, size_t size, gfp_t flags);
> >
> > NAK.
> >
> > This busticates some pretty subtle code in mm/slab.c that uses
> > uses __builtin_return_address() for debugging
> 
> Interesting. Here is output from kernel with patch applied and leak
> plugged into proc_dointvec() (I checked twice):
> 
>         $ grep kcalloc /proc/slab_allocators

Right.  That was the whole *point* of the patch to inline kcalloc - otherwise
you ended up with zillions of entries for kcalloc that didn't tell you where
they came from.

>         $ grep proc_dointvec /proc/slab_allocators
>         size-64: 19 proc_dointvec+0x48/0xa0

A lot more useful, no? ;)
<confoozled> I'm failing to see where proc_dointvec() ends up calling kcalloc?
So I'm not sure what that's

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Uninline kcalloc()

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Mon, 24 Sep 2007 11:44:35 +0400, Alexey Dobriyan said:

> Interesting. Here is output from kernel with patch applied and leak
> plugged into proc_dointvec() (I checked twice):
> 
>         $ grep kcalloc /proc/slab_allocators
>         $ grep proc_dointvec /proc/slab_allocators
>         size-64: 19 proc_dointvec+0x48/0xa0

Argh, went to "cancel" that previous msg and retry when I realized you meant it was
with *your* patch installed, managed to send it anyhow..

I wonder if the only reason it's working for you is because gcc is doing
some tail-recursion magic on the kcalloc() call (which calls to __kmalloc),
which just happens to make it work, and if the tail-recursion wasn't happening
it would be busticated.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Uninline kcalloc()

[Kyle Moffett]

On Sep 24, 2007, at 01:35:08, Valdis.Kletnieks@vt.edu wrote:
> On Sun, 23 Sep 2007 00:03:49 +0400, Alexey Dobriyan said:
>> -static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
>> -{
>> -	if (n != 0 && size > ULONG_MAX / n)
>> -		return NULL;
>> -	return __kmalloc(n * size, flags | __GFP_ZERO);
>> -}
>> +void *kcalloc(size_t n, size_t size, gfp_t flags);
>
> NAK.
>
> This busticates some pretty subtle code in mm/slab.c that uses uses  
> __builtin_return_address() for debugging - if you do this, then the  
> "calling function" gets listed as "kcalloc()" rather than the much  
> more useful "function that called kcalloc()" (which is what you  
> care about).
>
> (I remember going around and around multiple times getting those  
> stupid inlines set up right, so that feature actually did something  
> useful, otherwise kcalloc and kzalloc didn't report where they were  
> called from).

Proper fix is to give __kmalloc a "void *caller" parameter and have  
all of the various wrapper functions pass in the value of  
__builtin_return_address() appropriately.  I believe that even works  
properly for inline functions which may or may not be inlined.

Cheers,
Kyle Moffett

Re: [PATCH] Uninline kcalloc()

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Mon, 24 Sep 2007 08:59:56 EDT, Kyle Moffett said:

> Proper fix is to give __kmalloc a "void *caller" parameter and have  
> all of the various wrapper functions pass in the value of  
> __builtin_return_address() appropriately.  I believe that even works  
> properly for inline functions which may or may not be inlined.

I think I looked at that, and it wasn't as easy as it looked, because there
were ugly corner cases for what __builtin_return_address() returned depending
on exactly what did or didn't get inlined. Basically, it's ugly stuff - if you
try to use __builtin_retur_address inside kcalloc to pass to __kmalloc, and
then kcalloc gets inlined, you end up passing not the routine you wanted (the
caller of kcalloc), but the *parent* of that...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Uninline kcalloc()

[Christoph Lameter]

On Mon, 24 Sep 2007, Valdis.Kletnieks@vt.edu wrote:

> I think I looked at that, and it wasn't as easy as it looked, because there
> were ugly corner cases for what __builtin_return_address() returned depending
> on exactly what did or didn't get inlined. Basically, it's ugly stuff - if you
> try to use __builtin_retur_address inside kcalloc to pass to __kmalloc, and
> then kcalloc gets inlined, you end up passing not the routine you wanted (the
> caller of kcalloc), but the *parent* of that...

Note that this can be done in a clean way in SLUB using slab_alloc() which 
takes a address parameter as obtained from __builtin_return_address().