Re: [PATCH 1/3] cgroup: fix pre_destroy and semantics of css->refcnt

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

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 ?


Thanks,
-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>