On Thu 02-07-20 09:37:38, Roman Gushchin wrote:
On Thu, Jul 02, 2020 at 06:22:02PM +0200, Michal Hocko wrote:
On Wed 01-07-20 11:45:52, Roman Gushchin wrote: [...]
From c97afecd32c0db5e024be9ba72f43d22974f5bcd Mon Sep 17 00:00:00 2001
From: Roman Gushchin guro@fb.com Date: Wed, 1 Jul 2020 11:05:32 -0700 Subject: [PATCH] mm: kmem: make memcg_kmem_enabled() irreversible
Historically the kernel memory accounting was an opt-in feature, which could be enabled for individual cgroups. But now it's not true, and it's on by default both on cgroup v1 and cgroup v2. And as long as a user has at least one non-root memory cgroup, the kernel memory accounting is on. So in most setups it's either always on (if memory cgroups are in use and kmem accounting is not disabled), either always off (otherwise).
memcg_kmem_enabled() is used in many places to guard the kernel memory accounting code. If memcg_kmem_enabled() can reverse from returning true to returning false (as now), we can't rely on it on release paths and have to check if it was on before.
If we'll make memcg_kmem_enabled() irreversible (always returning true after returning it for the first time), it'll make the general logic more simple and robust. It also will allow to guard some checks which otherwise would stay unguarded.
Signed-off-by: Roman Gushchin guro@fb.com
mm/memcontrol.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 50ae77f3985e..2d018a51c941 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3582,7 +3582,8 @@ static int memcg_online_kmem(struct mem_cgroup *memcg) objcg->memcg = memcg; rcu_assign_pointer(memcg->objcg, objcg);
- static_branch_inc(&memcg_kmem_enabled_key);
- if (!memcg_kmem_enabled())
static_branch_inc(&memcg_kmem_enabled_key);Wouldn't be static_branch_enable() more readable?
Agree, will change, add reported-by and tested-by tags and resend. Thanks!
Feel free to add Acked-by: Michal Hocko mhocko@suse.com
Btw, don't we wanna to change memcg_kmem_enabled() definition from static_branch_unlikely() to static_branch_likely()?
Honestly, I do not know what would be the impact but considering kmem is enabled unless explicitly disabled these days then likely sounds more logical from reading POV. I do not think that early allocations until the first memcg is created is the case to optimize for. Worth a separate patch I guess.