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 _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
