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

[KAMEZAWA Hiroyuki]

On Tue, 02 Dec 2008 15:31:16 +0800
Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:

> KAMEZAWA Hiroyuki wrote:
> > On Tue, 02 Dec 2008 14:56:52 +0800
> > Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:
> > 
> >> KAMEZAWA Hiroyuki wrote:
> >>> On Tue, 02 Dec 2008 14:15:23 +0800
> >>> Li Zefan <lizf@xxxxxxxxxxxxxx> 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:
> 
but more complicated ;) Hmm...
Anyway , I'll queue some fix to next weekly-update.
Thank you for pointing out.

-Kame


> --- 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);
>  	}
>  }
>  
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/