lkml.org 
[lkml]   [2009]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH 2/4] memcg: fix error path of mem_cgroup_move_parent
    * Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> [2009-01-08 19:14:45]:

    > There is a bug in error path of mem_cgroup_move_parent.
    >
    > Extra refcnt got from try_charge should be dropped, and usages incremented
    > by try_charge should be decremented in both error paths:
    >
    > A: failure at get_page_unless_zero
    > B: failure at isolate_lru_page
    >
    > This bug makes this parent directory unremovable.
    >
    > In case of A, rmdir doesn't return, because res.usage doesn't go
    > down to 0 at mem_cgroup_force_empty even after all the pc in
    > lru are removed.
    > In case of B, rmdir fails and returns -EBUSY, because it has
    > extra ref counts even after res.usage goes down to 0.
    >
    >
    > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
    > ---
    > mm/memcontrol.c | 23 +++++++++++++++--------
    > 1 files changed, 15 insertions(+), 8 deletions(-)
    >
    > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
    > index 62e69d8..288e22c 100644
    > --- a/mm/memcontrol.c
    > +++ b/mm/memcontrol.c
    > @@ -983,14 +983,15 @@ static int mem_cgroup_move_account(struct page_cgroup *pc,
    > if (pc->mem_cgroup != from)
    > goto out;
    >
    > - css_put(&from->css);
    > res_counter_uncharge(&from->res, PAGE_SIZE);
    > mem_cgroup_charge_statistics(from, pc, false);
    > if (do_swap_account)
    > res_counter_uncharge(&from->memsw, PAGE_SIZE);
    > + css_put(&from->css);
    > +
    > + css_get(&to->css);
    > pc->mem_cgroup = to;
    > mem_cgroup_charge_statistics(to, pc, true);
    > - css_get(&to->css);
    > ret = 0;
    > out:
    > unlock_page_cgroup(pc);
    > @@ -1023,8 +1024,10 @@ static int mem_cgroup_move_parent(struct page_cgroup *pc,
    > if (ret || !parent)
    > return ret;
    >
    > - if (!get_page_unless_zero(page))
    > - return -EBUSY;
    > + if (!get_page_unless_zero(page)) {
    > + ret = -EBUSY;
    > + goto uncharge;
    > + }
    >
    > ret = isolate_lru_page(page);
    >
    > @@ -1033,19 +1036,23 @@ static int mem_cgroup_move_parent(struct page_cgroup *pc,
    >
    > ret = mem_cgroup_move_account(pc, child, parent);
    >
    > - /* drop extra refcnt by try_charge() (move_account increment one) */
    > - css_put(&parent->css);
    > putback_lru_page(page);
    > if (!ret) {
    > put_page(page);
    > + /* drop extra refcnt by try_charge() */
    > + css_put(&parent->css);
    > return 0;
    > }
    > - /* uncharge if move fails */
    > +
    > cancel:
    > + put_page(page);
    > +uncharge:
    > + /* drop extra refcnt by try_charge() */
    > + css_put(&parent->css);
    > + /* uncharge if move fails */
    > res_counter_uncharge(&parent->res, PAGE_SIZE);
    > if (do_swap_account)
    > res_counter_uncharge(&parent->memsw, PAGE_SIZE);
    > - put_page(page);
    > return ret;
    > }
    >
    >

    Looks good to me, just out of curiousity how did you catch this error?
    Through review or testing?

    --
    Balbir


    \
     
     \ /
      Last update: 2009-01-09 06:17    [W:0.029 / U:183.828 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site