Messages in this thread Patch in this message | | | Date | Tue, 02 Dec 2008 15:31:16 +0800 | From | Li Zefan <> | Subject | Re: [PATCH 1/3] cgroup: fix pre_destroy and semantics of css->refcnt |
| |
KAMEZAWA Hiroyuki wrote: > On Tue, 02 Dec 2008 14:56:52 +0800 > Li Zefan <lizf@cn.fujitsu.com> wrote: > >> KAMEZAWA Hiroyuki wrote: >>> On Tue, 02 Dec 2008 14:15:23 +0800 >>> Li Zefan <lizf@cn.fujitsu.com> wrote: >>> >>>> KAMEZAWA Hiroyuki wrote: >>>>> Now, final check of refcnt is done after pre_destroy(), so rmdir() can fail >>>>> after pre_destroy(). >>>>> memcg set mem->obsolete to be 1 at pre_destroy and this is buggy.. >>>>> >>>>> Several ways to fix this can be considered. This is an idea. >>>>> >>>> I don't see what's the difference with css_under_removal() in this patch and >>>> cgroup_is_removed() which is currently available. >>>> >>>> CGRP_REMOVED flag is set in cgroup_rmdir() when it's confirmed that rmdir can >>>> be sucessfully performed. >>>> >>>> So mem->obsolete can be replaced with: >>>> >>>> bool mem_cgroup_is_obsolete(struct mem_cgroup *mem) >>>> { >>>> return cgroup_is_removed(mem->css.cgroup); >>>> } >>>> >>>> Or am I missing something? >>>> >>> Yes. >>> 1. "cgroup" and "css" object are different object. >>> 2. css object may not be freed at destroy() (as current memcg does.) >>> >>> Some of css objects cannot be freed even when there are no tasks because >>> of reference from some persistent object or temporal refcnt. >>> >> I just noticed mem_cgroup has its own refcnt now. The memcg code has changed >> dramatically that I don't catch up with it. Thx for the explanation. >> >> But I have another doubt: >> >> void mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent) >> { >> struct mem_cgroup *memcg; >> >> memcg = __mem_cgroup_uncharge_common(page, >> MEM_CGROUP_CHARGE_TYPE_SWAPOUT); >> /* record memcg information */ >> if (do_swap_account && memcg) { >> swap_cgroup_record(ent, memcg); >> mem_cgroup_get(memcg); >> } >> } >> >> In the above code, is it possible that memcg is freed before mem_cgroup_get() >> increases memcg->refcnt? >> > Thank you for looking into. maybe possible. > > In this case, > 1. "the page" was belongs to memcg before uncharge(). > 2. but it's not guaranteed that memcg is alive after uncharge. > > OK. maybe css_tryget() can change this to be > == > rcu_read_lock(); > memcg = __mem_cgroup_uncharge_common(page, > MEM_CGROUP_CHARGE_TYPE_SWAPOUT); > if (do_swap_account && memcg && css_tryget(&memcg->css)) { > swap_cgroup_record(ent, memcg); > mem_cgroup_get(memcg); > css_put(&memcg->css); > } > rcu_read_unlock(); > == > How about this ? >
Seems OK for me. Another way to fix this is, don't call css_put() if we want to use the memcg returned from __mem_cgroup_uncharge_common(), I think this is more reasonable:
--- a/mm/memcontrol.c.orig 2008-12-02 15:20:55.000000000 +0800 +++ b/mm/memcontrol.c 2008-12-02 15:28:07.000000000 +0800 @@ -1110,8 +1110,9 @@ void mem_cgroup_cancel_charge_swapin(str /* * uncharge if !page_mapped(page) */ -static struct mem_cgroup * -__mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype) +static void +__mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype, + struct mem_cgroup **memcg) { struct page_cgroup *pc; struct mem_cgroup *mem = NULL; @@ -1163,13 +1164,16 @@ __mem_cgroup_uncharge_common(struct page mz = page_cgroup_zoneinfo(pc); unlock_page_cgroup(pc); - css_put(&mem->css); + /* don't dec refcnt, since the caller want to use this memcg */ + if (memcg) + *memcg = mem; + else + css_put(&mem->css); - return mem; + return; unlock_out: unlock_page_cgroup(pc); - return NULL; } void mem_cgroup_uncharge_page(struct page *page) @@ -1197,12 +1201,13 @@ void mem_cgroup_uncharge_swapcache(struc { struct mem_cgroup *memcg; - memcg = __mem_cgroup_uncharge_common(page, - MEM_CGROUP_CHARGE_TYPE_SWAPOUT); + __mem_cgroup_uncharge_common(page, + MEM_CGROUP_CHARGE_TYPE_SWAPOUT, &memcg); /* record memcg information */ if (do_swap_account && memcg) { swap_cgroup_record(ent, memcg); mem_cgroup_get(memcg); + css_put(&memcg->css); } }
| |