lkml.org 
[lkml]   [2006]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] fix memory leak in mm/slab.c::alloc_kmemlist() (try #2)
    Hi Pekka,

    On 3/19/06, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
    > On 3/18/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
    > > Currently the only caller of alloc_kmemlist() will BUG() if alloc_kmemlist()
    > > fails, but that doesn't mean we shouldn't clean up properly IMHO. Also, the
    > > caller (do_tune_cpucache()) could maybe be changed in the future to do
    > > something more clever than just BUG() and in that case we really shouldn't
    > > be leaking memory when we return -ENOMEM.
    >
    > Yeah, and BUG() can be no-op for embedded.
    >
    > On 3/18/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
    > > The patch has been compile and boot tested on x86, but since I'm not very
    > > intimate with the slab code I'd appreciate it if someone would take a close
    > > look on the changes before merging them.
    >
    > You probably didn't hit the error path on your x86 box. The patch
    > looks good to me for -mm although there's few comments below.
    >
    > > +/*
    > > + If one or more allocations fail we need to undo all allocations done up to
    > > + this point.
    > > + Unfortunately this means yet another loop, but since this only happens on
    > > + failure and frees up memory in a memory-tight situation, it's not too bad.
    > > + */
    >
    > The formatting of this comment looks strange.
    >
    > > + for_each_online_node(node) {
    > > + if (count <= 0)
    > > + break;
    > > + if (cachep->nodelists[node]) {
    >
    > Would probably make sense to extract the above expression into local
    > variable to reduce kernel text size.
    >
    > > + kfree(cachep->nodelists[node]->shared);
    > > + free_alien_cache(cachep->nodelists[node]->alien);
    > > + kfree(cachep->nodelists[node]);
    > > + cachep->nodelists[node] = NULL;
    > > + }
    > > + count--;
    > > + }
    > > + return -ENOMEM;
    >

    Thank you very much for your feedback.

    I'll create an updated patch with the changes you suggest. They make
    perfect sense.


    --
    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-03-20 09:58    [W:0.025 / U:0.020 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site