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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 Apr 2012 10:35:07 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	Tejun Heo <tj@...nel.org>
CC:	Glauber Costa <glommer@...allels.com>, netdev@...r.kernel.org,
	cgroups@...r.kernel.org, Li Zefan <lizefan@...wei.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>,
	Balbir Singh <bsingharora@...il.com>
Subject: Re: [PATCH v2 2/2] cgroup: get rid of populate for memcg

(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@...allels.com>
>> CC: Tejun Heo <tj@...nel.org>
>> CC: Li Zefan <lizefan@...wei.com>
>> ---
> ...
>> @@ -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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html