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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site