Re: [PATCH v2 2/2] cgroup: get rid of populate for memcg

[KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>]

(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 <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
> ...
>> @@ -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