Re: [PATCH 1/2] memcg: dirty pages accounting and limiting infrastructure

[KAMEZAWA Hiroyuki]

On Fri, 26 Feb 2010 14:53:39 +0900
Minchan Kim <minchan.kim@xxxxxxxxx> wrote:

> On Fri, Feb 26, 2010 at 2:01 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > On Fri, 26 Feb 2010 13:50:04 +0900
> > Minchan Kim <minchan.kim@xxxxxxxxx> wrote:
> >
> >> > Hm ? I don't read the whole thread but can_attach() is called under
> >> > cgroup_mutex(). So, it doesn't need to use RCU.
> >>
> >> Vivek mentioned memcg is protected by RCU if I understand his intention right.
> >> So I commented that without enough knowledge of memcg.
> >> After your comment, I dive into the code.
> >>
> >> Just out of curiosity.
> >>
> >> Really, memcg is protected by RCU?
> > yes. All cgroup subsystem is protected by RCU.
> >
> >> I think most of RCU around memcg is for protecting task_struct and
> >> cgroup_subsys_state.
> >> The memcg is protected by cgroup_mutex as you mentioned.
> >> Am I missing something?
> >
> > There are several levels of protections.
> >
> > cgroup subsystem's ->destroy() call back is finally called by
> >
> > As this.
> >
> > Â768 Â Â Â Â Â Â Â Â synchronize_rcu();
> > Â769
> > Â770 Â Â Â Â Â Â Â Â mutex_lock(&cgroup_mutex);
> > Â771 Â Â Â Â Â Â Â Â /*
> > Â772 Â Â Â Â Â Â Â Â Â* Release the subsystem state objects.
> > Â773 Â Â Â Â Â Â Â Â Â*/
> > Â774 Â Â Â Â Â Â Â Â for_each_subsys(cgrp->root, ss)
> > Â775 Â Â Â Â Â Â Â Â Â Â Â Â ss->destroy(ss, cgrp);
> > Â776
> > Â777 Â Â Â Â Â Â Â Â cgrp->root->number_of_cgroups--;
> > Â778 Â Â Â Â Â Â Â Â mutex_unlock(&cgroup_mutex);
> >
> > Before here,
> > Â Â Â Â- there are no tasks under this cgroup (cgroup's refcnt is 0)
> > Â Â Â Â Â&& cgroup is marked as REMOVED.
> >
> > Then, this access
> > Â Â Â Ârcu_read_lock();
> > Â Â Â Âmem = mem_cgroup_from_task(task);
> > Â Â Â Âif (css_tryget(mem->css)) Â <===============checks cgroup refcnt
> 
> If it it, do we always need css_tryget after mem_cgroup_from_task
> without cgroup_mutex to make sure css is vaild?
> 
On a case by cases. 

> But I found several cases that don't call css_tryget
> 
> 1. mm_match_cgroup
> It's used by page_referenced_xxx. so we I think we don't grab
> cgroup_mutex at that time.
> 
yes. but all check are done under RCU. And this function never
access contents of memory which pointer holds.
And, please conider the whole context.

	mem_cgroup_try_charge()
		mem_cout =..... (refcnt +1)
		....
		try_to_free_mem_cgroup_pages(mem_cont)
		....
		shrink_xxx_list()
		....
			page_referenced_anon(page, mem_cont)
				mm_match_cgroup(mm, mem_cont)

Then, this mem_cont (2nd argument to mm_match_cgroup) is always valid.
	rcu_read_lock();
	memcg = mem_cgroup_from_task(rcu_dereference(mm->ownder));
	rcu_read_unlock();
	return memcg != mem_cont;

Here,
	1. mem_cont is never reused. (because refcnt+1)
	2. we don't access memcg's contents.

I think even rcu_read_lock()/unlock() is unnecessary.



> 2. mem_cgroup_oom_called
> I think in here we don't grab cgroup_mutex, too.
> 
In OOM-killer context, memcg which causes OOM has refcnt +1.
Then, not necessary. 


> I guess some design would cover that problems.
Maybe.

> Could you tell me if you don't mind?
> Sorry for bothering you.
> 

In my point of view, the most terrible porblem is heavy cost of
css_tryget() when you run multi-thread heavy program.
So, I want to see some inovation, but haven't find yet.

I admit this RCU+refcnt is tend to be hard to review. But it's costly
operation to take refcnt and it's worth to be handled in the best
usage of each logics, on a case by cases for now.

Thanks,
-Kame


--
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/