From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [PATCH v2 2/2] cgroup: get rid of populate for memcg Date: Mon, 9 Apr 2012 10:40:42 -0700 Message-ID: <20120409174042.GA7522@google.com> References: <1333728250-14248-1-git-send-email-glommer@parallels.com> <1333728250-14248-3-git-send-email-glommer@parallels.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=JuY5R7Ji30iPYJDgWYHLGPPG6ESAcu4qZFB4EJRYEKA=; b=xIVSa1WPCpgDIQ908jYq01EUpD2PZ+3tV2piMIFuhCD1zYQVNpdl19n1jmu8zCUpqg LDgDu6t47LYTrP6kmw3OrsilZX4hDDreYPHB1J4vI+rtE8AagoCpRZ8z8R7Gb2LAeL6+ bHQlRCOnPS1WytA7zE9sJei+WrXYnH6l5DYJ1gktN5Lr405S0w6txMValXP6OJLCrf96 naL46/DBWh17wBwo0dBeW4o9JmL4WPdSjKGAbBbqLorSArLSZudoCKgn8kyGXMUCU0eG ZNpqgxaGtomJ1wD9ToOURhR9WgJZgOK4nGDcOGPd/inggivh4JAGBU0V2EhognUpm4jK knMA== Content-Disposition: inline In-Reply-To: <1333728250-14248-3-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Glauber Costa Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org, Li Zefan , Johannes Weiner , Michal Hocko , Balbir Singh (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. 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? Thanks. -- tejun