linux-kernel - Re: [PATCH 09/11] memcg: propagate kmem limiting information to children

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]

Message-ID: <4FE8E7EB.2020804@parallels.com>
Date:	Tue, 26 Jun 2012 02:36:27 +0400
From:	Glauber Costa <glommer@...allels.com>
To:	Tejun Heo <tj@...nel.org>
CC:	<cgroups@...r.kernel.org>, <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	<linux-kernel@...r.kernel.org>,
	"Frederic Weisbecker" <fweisbec@...il.com>,
	David Rientjes <rientjes@...gle.com>,
	"Pekka Enberg" <penberg@...nel.org>, Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>,
	Christoph Lameter <cl@...ux.com>, <devel@...nvz.org>,
	<kamezawa.hiroyu@...fujitsu.com>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Suleiman Souhlal <suleiman@...gle.com>
Subject: Re: [PATCH 09/11] memcg: propagate kmem limiting information to children

On 06/25/2012 10:29 PM, Tejun Heo wrote:
> Feeling like a nit pervert but..
>
> On Mon, Jun 25, 2012 at 06:15:26PM +0400, Glauber Costa wrote:
>> @@ -287,7 +287,11 @@ struct mem_cgroup {
>>   	 * Should the accounting and control be hierarchical, per subtree?
>>   	 */
>>   	bool use_hierarchy;
>> -	bool kmem_accounted;
>> +	/*
>> +	 * bit0: accounted by this cgroup
>> +	 * bit1: accounted by a parent.
>> +	 */
>> +	volatile unsigned long kmem_accounted;
>
> Is the volatile declaration really necessary?  Why is it necessary?
> Why no comment explaining it?

Seems to be required by set_bit and friends. gcc will complain if it is 
not volatile (take a look at the bit function headers)

>> +
>> +		for_each_mem_cgroup_tree(iter, memcg) {
>> +			struct mem_cgroup *parent;
>
> Blank line between decl and body please.
ok.

>
>> +			if (iter == memcg)
>> +				continue;
>> +			/*
>> +			 * We should only have our parent bit cleared if none of
>> +			 * ouri parents are accounted. The transversal order of
>
>                                ^ type
>
>> +			 * our iter function forces us to always look at the
>> +			 * parents.
>
> Also, it's okay here but the text filling in comments and patch
> descriptions tend to be quite inconsistent.  If you're on emacs, alt-q
> is your friend and I'm sure vim can do text filling pretty nicely too.
>
>> +			 */
>> +			parent = parent_mem_cgroup(iter);
>> +			while (parent && (parent != memcg)) {
>> +				if (test_bit(KMEM_ACCOUNTED_THIS, &parent->kmem_accounted))
>> +					goto noclear;
>> +					
>> +				parent = parent_mem_cgroup(parent);
>> +			}
>
> Better written in for (;;)?  Also, if we're breaking on parent ==
> memcg, can we ever hit NULL parent in the above loop?

I can simplify to test parent != memcg only, indeed it is not expected 
to be NULL (but if it happens to be due to any kind of bug, we protect 
against NULL-dereference, that is why I like to write this way)

>> +			continue;
>> +		}
>> +	}
>> +out:
>> +	mutex_unlock(&set_limit_mutex);
>
> Can we please branch on val != RECOURSE_MAX first?  I'm not even sure
> whether the above conditionals are correct.  If the user updates an
> existing kmem limit, the first test_and_set_bit() returns non-zero, so
> the code proceeds onto clearing KMEM_ACCOUNTED_THIS, which succeeds
> but val == RESOURCE_MAX fails so it doesn't do anything.  If the user
> changes it again, it will set ACCOUNTED_THIS again.  So, changing an
> existing kmem limit toggles KMEM_ACCOUNTED_THIS, which just seems
> wacky to me.
>

I will take a look at that tomorrow as well.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/