lkml.org 
[lkml]   [2011]   [Dec]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mm: memcg: keep root group unchanged if fail to create new
    On Sun, 11 Dec 2011, Hillf Danton wrote:

    > If the request is not to create root group and we fail to meet it,
    > we'd leave the root unchanged.

    I didn't understand that at first: please say "we should" rather
    than "we'd", which I take to be an abbreviation for "we would".

    >
    > Signed-off-by: Hillf Danton <dhillf@gmail.com>

    Yes indeed, well caught:
    Acked-by: Hugh Dickins <hughd@google.com>

    I wonder what was going through the author's mind when he wrote it
    that way? I wonder if it's one of those bugs that creeps in when
    you start from a perfectly functional patch, then make refinements
    to suit feedback from reviewers.

    On which topic: wouldn't this patch be better just to move the
    "root_mem_cgroup = memcg;" two lines lower down (and of course
    remove free_out's "root_mem_cgroup = NULL;" as you already did)?
    I can't see mem_cgroup_soft_limit_tree_init() relying on
    root_mem_cgroup at all.

    Should your patch go to stable, even to 2.6.32-longterm?
    Matter of taste, really: while it's not quite impossible for
    alloc_mem_cgroup_per_zone_info() to fail, it is very unlikely.

    > ---
    >
    > --- a/mm/memcontrol.c Fri Dec 9 21:57:40 2011
    > +++ b/mm/memcontrol.c Sun Dec 11 09:04:48 2011
    > @@ -4849,8 +4849,10 @@ mem_cgroup_create(struct cgroup_subsys *
    > enable_swap_cgroup();
    > parent = NULL;
    > root_mem_cgroup = memcg;
    > - if (mem_cgroup_soft_limit_tree_init())
    > + if (mem_cgroup_soft_limit_tree_init()) {
    > + root_mem_cgroup = NULL;
    > goto free_out;
    > + }
    > for_each_possible_cpu(cpu) {
    > struct memcg_stock_pcp *stock =
    > &per_cpu(memcg_stock, cpu);
    > @@ -4888,7 +4890,6 @@ mem_cgroup_create(struct cgroup_subsys *
    > return &memcg->css;
    > free_out:
    > __mem_cgroup_free(memcg);
    > - root_mem_cgroup = NULL;
    > return ERR_PTR(error);
    > }
    >


    \
     
     \ /
      Last update: 2011-12-12 00:41    [W:0.022 / U:31.336 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site