From mboxrd@z Thu Jan 1 00:00:00 1970 From: KAMEZAWA Hiroyuki Subject: Re: [PATCH v2 2/2] cgroup: get rid of populate for memcg Date: Tue, 10 Apr 2012 10:35:07 +0900 Message-ID: <4F838E4B.9080905@jp.fujitsu.com> References: <1333728250-14248-1-git-send-email-glommer@parallels.com> <1333728250-14248-3-git-send-email-glommer@parallels.com> <20120409174042.GA7522@google.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120409174042.GA7522-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" To: Tejun Heo Cc: Glauber Costa , netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Li Zefan , Johannes Weiner , Michal Hocko , Balbir Singh (2012/04/10 2:40), Tejun Heo wrote: > (cc'ing other memcg ppl just in case) > > Hello, > > I don't think the error handling is correct here. > > On Fri, Apr 06, 2012 at 08:04:10PM +0400, Glauber Costa wrote: >> The last man standing justifying the need for populate() is the >> sock memcg initialization functions. Now that we are able to pass >> a struct mem_cgroup instead of a struct cgroup to the socket >> initialization, there is nothing that stops us from initializing >> everything in create(). >> >> Signed-off-by: Glauber Costa >> CC: Tejun Heo >> CC: Li Zefan >> --- > ... >> @@ -5010,7 +5010,9 @@ mem_cgroup_create(struct cgroup *cont) >> memcg->move_charge_at_immigrate = 0; >> mutex_init(&memcg->thresholds_lock); >> spin_lock_init(&memcg->move_lock); >> - return &memcg->css; >> + >> + if (!memcg_init_kmem(memcg, &mem_cgroup_subsys)) >> + return &memcg->css; >> free_out: >> __mem_cgroup_free(memcg); >> return ERR_PTR(error); > > So, the control is just falling through free_out: on kmem init > failure; however, there seem to be stuff which needs to be undone - > hotcpu_notifier() registration, which BTW seems incorrect even on its > own - unmounting and mounting again would probably make the same > notifier registered multiple times corrupting notification chain, and > ref inc on the parent. > ok, it should be fixed. > It probably would be best to reorganize the function slightly such > that, it's organized as... > > 1. alloc > 2. init stuff w/o other side effects > 3. make side effects > > and add kmemcg init at the end of the second step. > > Also, memcg maintainers, once the patches get updated and acked, I'd > like to route them through cgroup tree so that I can kill ->populate > there. cgroup/for-3.5 is stable branch which can be pulled into other > trees including the memcg one. Would that be okay? > Hm, I'm okay with that but.....Michal ? Thanks, -Kame