A few comments on your patch below.
On 5/13/06, Catalin Marinas <[email protected]> wrote:
From: Catalin Marinas <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
