2007-09-22 20:04:23

by Alexey Dobriyan

[permalink] [raw]
Subject: [PATCH] Uninline kcalloc()

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

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


2007-09-24 05:35:52

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

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


Attachments:
(No filename) (226.00 B)

2007-09-24 06:22:40

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()


On Sep 24 2007 01:35, [email protected] 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?

2007-09-24 07:24:28

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

On Mon, 24 Sep 2007 08:22:31 +0200, Jan Engelhardt said:
>
> On Sep 24 2007 01:35, [email protected] 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....


Attachments:
(No filename) (226.00 B)

2007-09-24 07:44:44

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

On 9/24/07, [email protected] <[email protected]> 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

2007-09-24 07:55:30

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

On Mon, 24 Sep 2007 11:44:35 +0400, Alexey Dobriyan said:
> On 9/24/07, [email protected] <[email protected]> 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


Attachments:
(No filename) (226.00 B)

2007-09-24 08:13:33

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

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.


Attachments:
(No filename) (226.00 B)

2007-09-24 13:01:20

by Kyle Moffett

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

On Sep 24, 2007, at 01:35:08, [email protected] 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

2007-09-24 13:17:47

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

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


Attachments:
(No filename) (226.00 B)

2007-09-25 21:21:41

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] Uninline kcalloc()

On Mon, 24 Sep 2007, [email protected] 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().