linux-kernel - Re: [PATCH] memcg: remove KMEM_ACCOUNTED

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: <CAA6-i6rWsZNQmFY5L-=yc6TaTGyg4hP4qn9gMZVsu8wWJ=1ywg@mail.gmail.com>
Date:	Mon, 2 Dec 2013 22:26:48 +0400
From:	Glauber Costa <glommer@...il.com>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	Vladimir Davydov <vdavydov@...allels.com>,
	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 Mon, Dec 2, 2013 at 10:15 PM, Michal Hocko <mhocko@...e.cz> wrote:
> [CCing Glauber - please do so in other posts for kmem related changes]
>
> On Mon 02-12-13 17:08:13, Vladimir Davydov wrote:
>> The KMEM_ACCOUNTED_ACTIVATED was introduced by commit a8964b9b ("memcg:
>> use static branches when code not in use") in order to guarantee that
>> static_key_slow_inc(&memcg_kmem_enabled_key) will be called only once
>> for each memory cgroup when its kmem limit is set. The point is that at
>> that time the memcg_update_kmem_limit() function's workflow looked like
>> this:
>>
>>       bool must_inc_static_branch = false;
>>
>>       cgroup_lock();
>>       mutex_lock(&set_limit_mutex);
>>       if (!memcg->kmem_account_flags && val != RESOURCE_MAX) {
>>               /* The kmem limit is set for the first time */
>>               ret = res_counter_set_limit(&memcg->kmem, val);
>>
>>               memcg_kmem_set_activated(memcg);
>>               must_inc_static_branch = true;
>>       } else
>>               ret = res_counter_set_limit(&memcg->kmem, val);
>>       mutex_unlock(&set_limit_mutex);
>>       cgroup_unlock();
>>
>>       if (must_inc_static_branch) {
>>               /* We can't do this under cgroup_lock */
>>               static_key_slow_inc(&memcg_kmem_enabled_key);
>>               memcg_kmem_set_active(memcg);
>>       }
>>
>> Today, we don't use cgroup_lock in memcg_update_kmem_limit(), and
>> static_key_slow_inc() is called under the set_limit_mutex, but the
>> leftover from the above-mentioned commit is still here. Let's remove it.
>
> OK, so I have looked there again and 692e89abd154b (memcg: increment
> static branch right after limit set) which went in after cgroup_mutex
> has been removed. It came along with the following comment.
>                 /*
>                  * setting the active bit after the inc will guarantee no one
>                  * starts accounting before all call sites are patched
>                  */
>
> This suggests that the flag is needed after all because we have
> to be sure that _all_ the places have to be patched. AFAIU
> memcg_kmem_newpage_charge might see the static key already patched so
> it would do a charge but memcg_kmem_commit_charge would still see it
> unpatched and so the charge won't be committed.
>
> Or am I missing something?

You are correct. This flag is there due to the way we are using static branches.
The patching of one call site is atomic, but the patching of all of
them are not.
Therefore we need to use a two-flag scheme to guarantee that in the first time
we turn the static branches on, there will be a clear point after
which we're going
to start accounting.





-- 
E Mare, Libertas
--
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/