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

[Jesper Juhl]

Hi Pekka,

On 3/19/06, Pekka Enberg <penberg@xxxxxxxxxxxxxx> wrote:
> On 3/18/06, Jesper Juhl <jesper.juhl@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/