Messages in this thread |  | | | Date | Sat, 13 May 2006 19:42:40 +0200 | | From | "Jesper Juhl" <> | | Subject | Re: [PATCH 2.6.17-rc4 1/6] Base support for kmemleak |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |