lkml.org 
[lkml]   [2006]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-05-13 19:45    [W:0.051 / U:1.860 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site