mm: memcontrol: treat disabled memcg as kmem accounting disabled
Subsystems (3)
PATCH 1/1 mm: memcontrol: treat disabled memcg as kmem accounting disabled
mem_cgroup_kmem_disabled() currently only checks whether the "cgroup.memory=nokmem" option is specified. However, kmem accounting is also unavailable when memcg itself is disabled.
Check both conditions to ensure the function accurately reflects the kmem accounting state.
--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
@@ -132,7 +132,7 @@ static DEFINE_SPINLOCK(objcg_lock);bool mem_cgroup_kmem_disabled (void ) { -return cgroup_memory_nokmem; +return cgroup_memory_nokmem ||mem_cgroup_disabled (); }static void memcg_uncharge (struct mem_cgroup *memcg,unsigned int nr_pages);
Signature · 1 line
2.54.0
Quoted 6 lines · Hao Li
mem_cgroup_kmem_disabled()currently only checks whether the "cgroup.memory=nokmem" option is specified. However, kmem accounting is also unavailable when memcg itself is disabled.Check both conditions to ensure the function accurately reflects the kmem accounting state.
It would be really great if you could describe how we could end up with the inconsistent memcg enabled but kmem enabled and what kind of effect does this have.
AFAICS the inconsistency is possible and it would lead some wastage but no functional problems but the changelog should be more descriptive.
Quoted 20 lines
Signed-off-by Hao Li <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +1
-1@@ -132,7 +132,7 @@ static DEFINE_SPINLOCK(objcg_lock);bool mem_cgroup_kmem_disabled (void ) { -return cgroup_memory_nokmem; +return cgroup_memory_nokmem ||mem_cgroup_disabled (); }static void memcg_uncharge (struct mem_cgroup *memcg,unsigned int nr_pages);Signature · 1 line
2.54.0
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 11 lines · Michal Hocko
Quoted 6 lines · Hao Li
mem_cgroup_kmem_disabled()currently only checks whether the "cgroup.memory=nokmem" option is specified. However, kmem accounting is also unavailable when memcg itself is disabled.Check both conditions to ensure the function accurately reflects the kmem accounting state.
It would be really great if you could describe how we could end up with the inconsistent memcg enabled but kmem enabled and what kind of effect does this have.
Yes, thanks for point out this.
Quoted 3 lines
AFAICS the inconsistency is possible and it would lead some wastage but no functional problems but the changelog should be more descriptive.
Exactly! The most direct benefit is that when memcg is disabled, new_kmalloc_cache() will not need to create a separate KMALLOC_CGROUP slub cache, but can simply alias it to KMALLOC_NORMAL. This avoids wastage.
If this sounds reasonable, I would be happy to explain it in more detail in v2.
Quoted 25 lines
Quoted 21 lines
Signed-off-by Hao Li <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +1
-1@@ -132,7 +132,7 @@ static DEFINE_SPINLOCK(objcg_lock);bool mem_cgroup_kmem_disabled (void ) { -return cgroup_memory_nokmem; +return cgroup_memory_nokmem ||mem_cgroup_disabled (); }static void memcg_uncharge (struct mem_cgroup *memcg,unsigned int nr_pages);Signature · 1 line
2.54.0Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Thanks, Hao
Quoted 16 lines · Hao Li
Quoted 11 lines · Michal Hocko
Quoted 6 lines · Hao Li
mem_cgroup_kmem_disabled()currently only checks whether the "cgroup.memory=nokmem" option is specified. However, kmem accounting is also unavailable when memcg itself is disabled.Check both conditions to ensure the function accurately reflects the kmem accounting state.
It would be really great if you could describe how we could end up with the inconsistent memcg enabled but kmem enabled and what kind of effect does this have.
Yes, thanks for point out this.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
AFAICS the inconsistency is possible and it would lead some wastage but no functional problems but the changelog should be more descriptive.
Exactly! The most direct benefit is that when memcg is disabled,
new_kmalloc_cache()will not need to create a separateKMALLOC_CGROUPslub cache, but can simply alias it toKMALLOC_NORMAL. This avoids wastage.
This is definitely important detail to mention in the chagelog. Same as the effect on the __list_lru_init and other callers. TBH I am no longer 100% sure this is correct. You need to explain more why this is just wastage rathe than a subtle side effect that is desirable.
Quoted 32 lines
If this sounds reasonable, I would be happy to explain it in more detail in v2.
Quoted 25 lines
Quoted 21 lines
Signed-off-by Hao Li <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +1
-1@@ -132,7 +132,7 @@ static DEFINE_SPINLOCK(objcg_lock);bool mem_cgroup_kmem_disabled (void ) { -return cgroup_memory_nokmem; +return cgroup_memory_nokmem ||mem_cgroup_disabled (); }static void memcg_uncharge (struct mem_cgroup *memcg,unsigned int nr_pages);Signature · 1 line
2.54.0Signature · 2 lines
Michal Hocko SUSE LabsSignature · 2 lines
Thanks, Hao
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 20 lines · Michal Hocko
Quoted 19 lines · Hao Li
Quoted 11 lines · Michal Hocko
Quoted 6 lines · Hao Li
mem_cgroup_kmem_disabled()currently only checks whether the "cgroup.memory=nokmem" option is specified. However, kmem accounting is also unavailable when memcg itself is disabled.Check both conditions to ensure the function accurately reflects the kmem accounting state.
It would be really great if you could describe how we could end up with the inconsistent memcg enabled but kmem enabled and what kind of effect does this have.
Yes, thanks for point out this.
Quoted 3 lines
AFAICS the inconsistency is possible and it would lead some wastage but no functional problems but the changelog should be more descriptive.
Quoted 8 lines
Quoted 3 lines
Exactly! The most direct benefit is that when memcg is disabled,
new_kmalloc_cache()will not need to create a separateKMALLOC_CGROUPslub cache, but can simply alias it toKMALLOC_NORMAL. This avoids wastage.This is definitely important detail to mention in the chagelog. Same as the effect on the
__list_lru_initand other callers. TBH I am no longer 100% sure this is correct. You need to explain more why this is just wastage rathe than a subtle side effect that is desirable.
That's a very fair question. Let me walk through the impact of this patch across the various subsystems in detail below.
In fact, the initial motivation for this patch stemmed from a slab patch
discussion: https://lore.kernel.org/linux-mm/amHldYIo_-Bgm7Ek@fedora/, so also Cc'ing the slab folks. But in retrospect, its impact on list_lru is somewhat more subtle.
1. Impact on list_lru (the most widely affected path)
a. list_lru_register and list_lru_unregister previously performed pointless memcg_list_lrus operations. With this patch, both functions become no-ops.
b. In list_lru_from_memcg_idx, previously, because memcg was disabled, any input idx reaching this function was guaranteed to be -1, so the if branch was never taken. With this patch, the if branch is still not taken, so the control flow remains unaffected.
c. In list_lru_add_obj / list_lru_del_obj, they previously performed some unnecessary RCU operations in the if branch, and mem_cgroup_from_virt returned NULL, meaning there was essentially no difference between the if and else branches. With this patch, they directly take the else branch.
d. In list_lru_walk_node, before this patch, because memcg was disabled, there were no elements in lru->xa. The function had already completed its task once list_lru_walk_one finished, making the subsequent xa_for_each inside the if block effectively a no-op. With this patch, these pointless no-ops are skipped directly at the if check.
e. memcg_destroy_list_lru was previously a no-op as well because lru->xa contained no elements. With this patch, the function bails out early, causing no functional change.
f. memcg_list_lru_alloc / folio_memcg_list_lru_alloc were previously unreachable because memcg was disabled or objcg was NULL, so applying this patch has no impact here either.
g. memcg_init_list_lru previously initialized lru->xa. With this patch, it is no longer initialized. This is safe because all paths attempting to access lru->xa are either guarded by list_lru_memcg_aware(), or become no-ops due to memcg_list_lrus being empty.
In summary, the impact of this patch on list_lru either preserves the existing execution flow or saves unnecessary operations, introducing no adverse side effects.
Additionally, when shrinker_memcg_alloc detects that memcg is disabled, it returns -ENOSYS to let shrinker_alloc clear the SHRINKER_MEMCG_AWARE flag. This confirms that the shrinker does not care about memcg list_lru when memcg is disabled, further corroborating that this patch aligns with the shrinker's design rationale.
2. Impact on slab KMALLOC_CGROUP is aliased to KMALLOC_NORMAL instead of getting its own set of caches, and __kmem_cache_create_args no longer sets SLAB_MAY_ACCOUNT, eliminating the need to allocate the obj_cgroup vector.
3. Impact on need_pcpuobj_ext and pcpu_obj_full_size When CONFIG_MEM_ALLOC_PROFILING=n, there is no longer a need to allocate pcpuobj_ext. pcpu_obj_full_size is not affected as objcg no longer exist when kmem account is disabled.
4. Impact on memcg_online_kmem / memcg_offline_kmem memcg_offline_kmem is unreachable when memcg is disabled, and memcg_online_kmem will bail out at mem_cgroup_kmem_disabled, so memcg_kmem_online_key is still not set.
Finally, with CONFIG_MEMCG=n, mem_cgroup_disabled() and mem_cgroup_kmem_disabled() both return true. Therefore, making mem_cgroup_kmem_disabled() also return true under CONFIG_MEMCG=y with cgroup_disable=memory seems reasonable and consistent.
Untangling all the combinations of whether memcg and kmem accounting are enabled is surprisingly intricate! Please feel free to point it out if I've missed anything.
Quoted 37 lines
Quoted 33 lines
If this sounds reasonable, I would be happy to explain it in more detail in v2.
Quoted 25 lines
Quoted 21 lines
Signed-off-by Hao Li <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memcontrol.c +1
-1@@ -132,7 +132,7 @@ static DEFINE_SPINLOCK(objcg_lock);bool mem_cgroup_kmem_disabled (void ) { -return cgroup_memory_nokmem; +return cgroup_memory_nokmem ||mem_cgroup_disabled (); }static void memcg_uncharge (struct mem_cgroup *memcg,unsigned int nr_pages);Signature · 1 line
2.54.0Signature · 2 lines
Michal Hocko SUSE LabsSignature · 2 lines
Thanks, HaoSignature · 2 lines
Michal Hocko SUSE Labs