Message-ID: <529EDB41.8030505@parallels.com>
Date:	Wed, 4 Dec 2013 11:35:29 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	Glauber Costa <glommer@...il.com>
CC:	Michal Hocko <mhocko@...e.cz>, LKML <linux-kernel@...r.kernel.org>,
	<cgroups@...r.kernel.org>, <devel@...nvz.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Balbir Singh <bsingharora@...il.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH] memcg: remove KMEM_ACCOUNTED_ACTIVATED

On 12/04/2013 02:38 AM, Glauber Costa wrote:
>> In memcg_update_kmem_limit() we do the whole process of limit
>> initialization under a mutex so the situation we need protection from in
>> tcp_update_limit() is impossible. BTW once set, the 'activated' flag is
>> never cleared and never checked alone, only along with the 'active'
>> flag, that's why I doubt we need it at all.
> The updates are protected by a mutex, but the readers are not, and should not.
> So we can still be patching the readers, and the double-flag was
> initially used so
> we can make sure that both flags are only set after the static branches are in.
>
> Note that one of the flags is set inside memcg_update_cache_sizes(). After that,
> we call static_key_slow_inc(). At this point, someone whose code is
> patched in could
> start accounting, but it shouldn't - because not all sites are patched in.
>
> So after the update is done, we set the other flag, and now everybody
> will start going
> through.
>
> Could you do something clever with just one flag? Probably yes. But I
> doubt it would
> be that much cleaner, this is just the way that patching sites work.

Thank you for spending your time to listen to me.

Let me try to explain what is bothering me.

We have two state bits for each memcg, 'active' and 'activated'. There
are two scenarios where the bits can be modified:

1) The kmem limit is set on a memcg for the first time -
memcg_update_kmem_limit(). Here we call memcg_update_cache_sizes(),
which sets the 'activated' bit on success, then update static branching,
then set the 'active' bit. All three actions are done atomically in
respect to other tasks setting the limit due to the set_limit_mutex.
After both bits are set, they never get cleared for the memcg.

2) When a subgroup of a kmem-active cgroup is created -
memcg_propagate_kmem(). Here we copy kmem_account_flags from the parent,
then increase static branching refcounter, then call
memcg_update_cache_sizes() for the new memcg, which may clear the
'activated' bit on failure. After successful execution, the state bits
never get cleared for the new memcg.

In scenario 2 there is no need bothering about the flags setting order,
because we don't have any tasks in the cgroup yet - the tasks can be
moved in only after css_online finishes when we have both of the bits
set and the static branching enabled. Actually, we already do not bother
about it, because we have both bits set before the cgroup is fully
initialized (memcg_update_cache_sizes() is called).

Let's look at scenario 1. There we have two bits - 'activated' and
'active' - the latter is always set after the former and after the
static branching is enabled. Moreover, none of the bits is cleared once
it's set. That said checking if both bits are set - I mean
memcg_can_account_kmem() - is equivalent to checking if the 'acitve' bit
is set. Next, the 'activated' bit is never checked alone, only along
with the 'active' bit in memcg_can_account_kmem() - I do not count
(!memcg->kmem_account_flags) check in memcg_update_kmem_limit(), because
it is done under the set_limit_mutex. What's the deal having it then?

Thanks and sorry for disturbing you.
--
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/

Powered by blists - more mailing lists