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

    Hi Pekka, Andrew, Christoph & everyone else,


    On Monday 20 March 2006 09:55, Jesper Juhl wrote:
    > 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.
    >

    Here's the latest version of my patch to fix the mem leak in alloc_kmemlist().
    It should address Pekkas's comments.

    Andrew: Do you think this could go into -mm and get some field testing, so
    perhaps it has a chance of making 2.6.17?


    Fix memory leak in mm/slab.c::alloc_kmemlist().
    If one allocation fails we have to roll-back all allocations made up to the
    point of failure.


    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
    ---

    mm/slab.c | 36 ++++++++++++++++++++++++++++++------
    1 files changed, 30 insertions(+), 6 deletions(-)

    --- linux-2.6.16-rc6-mm2-orig/mm/slab.c 2006-03-18 16:55:55.000000000 +0100
    +++ linux-2.6.16-rc6-mm2/mm/slab.c 2006-03-21 22:33:45.000000000 +0100
    @@ -3399,12 +3399,17 @@ EXPORT_SYMBOL_GPL(kmem_cache_name);
    static int alloc_kmemlist(struct kmem_cache *cachep)
    {
    int node;
    + int count = -1;
    struct kmem_list3 *l3;
    - int err = 0;
    + struct array_cache *new;
    + struct array_cache **new_alien;

    for_each_online_node(node) {
    - struct array_cache *nc = NULL, *new;
    - struct array_cache **new_alien = NULL;
    + struct array_cache *nc = NULL;
    +
    + new = NULL;
    + new_alien = NULL;
    + count++;
    #ifdef CONFIG_NUMA
    new_alien = alloc_alien_cache(node, cachep->limit);
    if (!new_alien)
    @@ -3447,10 +3452,29 @@ static int alloc_kmemlist(struct kmem_ca
    cachep->batchcount + cachep->num;
    cachep->nodelists[node] = l3;
    }
    - return err;
    + return 0;
    +/**
    + * 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.
    + */
    fail:
    - err = -ENOMEM;
    - return err;
    + kfree(new);
    + free_alien_cache(new_alien);
    + for_each_online_node(node) {
    + if (count <= 0)
    + break;
    + l3 = cachep->nodelists[node];
    + if (l3) {
    + kfree(l3->shared);
    + free_alien_cache(l3->alien);
    + kfree(l3);
    + cachep->nodelists[node] = NULL;
    + }
    + count--;
    + }
    + return -ENOMEM;
    }

    struct ccupdate_struct {



    -
    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-22 01:56    [W:0.037 / U:0.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site