(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 <glommer@xxxxxxxxxxxxx> > CC: Tejun Heo <tj@xxxxxxxxxx> > CC: Li Zefan <lizefan@xxxxxxxxxx> > --- ... > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
