Re: [PATCH 2.6.17-rc4 1/6] Base support for kmemleak

Jesper Juhl
2006-05-13T17:42:42Z

A few comments on your patch below.

On 5/13/06, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> From: Catalin Marinas <catalin.marinas@arm.com>
>
> This patch adds the base support for the kernel memory leak detector. It
> traces the memory allocation/freeing in a way similar to the Boehm's
> conservative garbage collector, the difference being that the orphan
> pointers are not freed but only shown in /proc/memleak. Enabling this

/proc is such a mess already, do we have to add another file to it?
How about using sysfs instead? I know that is "one value pr file", but
then simply make one file pr leaked pointer or something like that...

> feature would introduce an overhead to memory allocations.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>
> include/linux/kernel.h | 13 +
> include/linux/memleak.h | 55 +++++
> init/main.c | 5
> lib/Kconfig.debug | 11 +
> mm/Makefile | 2
> mm/memleak.c | 549 +++++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 632 insertions(+), 3 deletions(-)
>
[snip]
> diff --git a/include/linux/memleak.h b/include/linux/memleak.h
[snip]
> +#define memleak_offsetof(type, member) \
> + (__builtin_constant_p(&((type *) 0)->member) ? \
> + ((size_t) &((type *) 0)->member) : 0)
> +

No spaces after the closing parenthesis of a cast and the value being
cast please.

(__builtin_constant_p(&((type *)0)->member) ? \
((size_t) &((type *)0)->member) : 0)

There are more occourances of this, only pointing out the first one.

[snip]
[snip]
> +config DEBUG_MEMLEAK
> + bool "Kernel memory leak detector"
> + depends on DEBUG_KERNEL && SLAB
> + help
> + Say Y here if you want to enable the memory leak
> + detector. The memory allocation/freeing is traced in a way
> + similar to the Boehm's conservative garbage collector, the
> + difference being that the orphan pointers are not freed but
> + only shown in /proc/memleak. Enabling this feature would
> + introduce an overhead to memory allocations.

Shouldn't that last bit read "Enabling this feature will introduce
overhead to memory allocations." ?

[snip]
> +#define MAX_TRACE 1
> +#endif
> +
> +
> +extern struct memleak_offset __memleak_offsets_start[];
> +extern struct memleak_offset __memleak_offsets_end[];
> +
> +
> +struct memleak_alias {

You seem to be very fond of double empty lines, here and elsewhere.
Surely just a single blank line would do just fine in many places -
no?

[snip]
> +static inline void delete_pointer(unsigned long ptr)

"inline" ? Isn't this function a little too fat for that?

[snip]
> +/* Freeing function hook
> + */

A lot of lines could be saved if all these small comments were on a
single line instead...

/* Freeing function hook */

[snip]
> + delete_pointer((unsigned long) ptr);

delete_pointer((unsigned long)ptr);

[snip]
> +static void memleak_scan(void)
> +{
> + unsigned long flags;
> + struct memleak_pointer *pointer;
> + struct task_struct *task;
> + int node;
> +#ifdef CONFIG_SMP
> + int i;
> +#endif

Why not just get rid of `i' and just use the `node' variable in the
single location where `i' is used (or get rid of `node' and use `i' in
its place) ?
As far as I can see that shouldn't be a problem and it'll save one
local variable on SMP.

[snip]
> + memleak_scan_block((void *) pointer->pointer,
> + (void *) (pointer->pointer + pointer->size));

memleak_scan_block((void *)pointer->pointer,
(void *)(pointer->pointer + pointer->size));

[snip]
> +static void *memleak_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> +{
> + struct list_head *n = ((struct memleak_pointer *) v)->pointer_list.next;

struct list_head *n = ((struct memleak_pointer *)v)->pointer_list.next;

> +
> + ++(*pos);
> +
> + return (n != &pointer_list)
> + ? list_entry(n, struct memleak_pointer, pointer_list)
> + : NULL;

Wouldn't this be more readable as

if (n != &pointer_list)
return list_entry(n, struct memleak_pointer, pointer_list);
return NULL

???

[snip]
> +int __init memleak_init(void)
> +{
> + struct memleak_offset *ml_off;
> + int aliases = 0;
> + unsigned long flags;
> +
> + printk(KERN_INFO "Kernel memory leak detector\n");

How about moving this printk() to the end of memleak_init() and changing it to :

printk(KERN_INFO "Kernel memory leak detector initialized.\n");

[snip]
> +#if 0
> + /* make some orphan pointers for testing */
> + kmalloc(32, GFP_KERNEL);
> + kmalloc(32, GFP_KERNEL);
> + kmem_cache_alloc(pointer_cache, GFP_ATOMIC);
> + kmem_cache_alloc(pointer_cache, GFP_ATOMIC);
> + vmalloc(64);
> + vmalloc(64);
> +#endif

Stuff for testing is nice, but do we have to add it to the kernel? - I
assume you are done testing :-)
We have waay too much code already in the kernel inside #if 0

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html