Re: [PATCH] fix memory leak in mm/slab.c::alloc_kmemlist() (try #2)

["Pekka Enberg" <penberg@cs.helsinki.fi>]

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;
-
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/