[v2] mm: kmem: make memcg_kmem_enabled() irreversible

Commit Message

Roman Gushchin July 2, 2020, 6:09 p.m. UTC

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 <[e-mail]>
Reported-by: Naresh Kamboju <[e-mail]>
Tested-by: Naresh Kamboju <[e-mail]>
Reviewed-by: Shakeel Butt <[e-mail]>
Acked-by: Vlastimil Babka <[e-mail]>
Acked-by: Michal Hocko <[e-mail]>
---
 mm/memcontrol.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Roman Gushchin July 2, 2020, 6:17 p.m. UTC | #1

On Thu, Jul 02, 2020 at 11:09:26AM -0700, Roman Gushchin wrote:
> 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 <[e-mail]>
> Reported-by: Naresh Kamboju <[e-mail]>
> Tested-by: Naresh Kamboju <[e-mail]>
> Reviewed-by: Shakeel Butt <[e-mail]>
> Acked-by: Vlastimil Babka <[e-mail]>
> Acked-by: Michal Hocko <[e-mail]>

Andrew,

can you, please, put this patch before my slab series?

Thank you!

Roman

Patch

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 50ae77f3985e..0145a77aa074 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);
+	static_branch_enable(&memcg_kmem_enabled_key);
+
 	/*
 	 * A memory cgroup is considered kmem-online as soon as it gets
 	 * kmemcg_id. Setting the id after enabling static branching will
@@ -3643,9 +3644,6 @@  static void memcg_free_kmem(struct mem_cgroup *memcg)
 	/* css_alloc() failed, offlining didn't happen */
 	if (unlikely(memcg->kmem_state == KMEM_ONLINE))
 		memcg_offline_kmem(memcg);
-
-	if (memcg->kmem_state == KMEM_ALLOCATED)
-		static_branch_dec(&memcg_kmem_enabled_key);
 }
 #else
 static int memcg_online_kmem(struct mem_cgroup *memcg)