Re: [PATCH v2 07/28] mm: memcontrol: return root object cgroup for root memory cgroup

[Chen Ridong <chenridong@huaweicloud.com>]

On 2025/12/26 11:10, Muchun Song wrote:
> 
> 
> On 2025/12/26 09:03, Chen Ridong wrote:
>>
>> On 2025/12/17 15:27, Qi Zheng wrote:
>>> From: Muchun Song <songmuchun@bytedance.com>
>>>
>>> Memory cgroup functions such as get_mem_cgroup_from_folio() and
>>> get_mem_cgroup_from_mm() return a valid memory cgroup pointer,
>>> even for the root memory cgroup. In contrast, the situation for
>>> object cgroups has been different.
>>>
>>> Previously, the root object cgroup couldn't be returned because
>>> it didn't exist. Now that a valid root object cgroup exists, for
>>> the sake of consistency, it's necessary to align the behavior of
>>> object-cgroup-related operations with that of memory cgroup APIs.
>>>
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>>> ---
>>>   include/linux/memcontrol.h | 26 +++++++++++++++++-----
>>>   mm/memcontrol.c            | 45 ++++++++++++++++++++------------------
>>>   mm/percpu.c                |  2 +-
>>>   3 files changed, 45 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index 288dd6337f80f..776d9be1f446a 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -332,6 +332,7 @@ struct mem_cgroup {
>>>   #define MEMCG_CHARGE_BATCH 64U
>>>     extern struct mem_cgroup *root_mem_cgroup;
>>> +extern struct obj_cgroup *root_obj_cgroup;
>>>     enum page_memcg_data_flags {
>>>       /* page->memcg_data is a pointer to an slabobj_ext vector */
>>> @@ -549,6 +550,11 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
>>>       return (memcg == root_mem_cgroup);
>>>   }
>>>   +static inline bool obj_cgroup_is_root(const struct obj_cgroup *objcg)
>>> +{
>>> +    return objcg == root_obj_cgroup;
>>> +}
>>> +
>>>   static inline bool mem_cgroup_disabled(void)
>>>   {
>>>       return !cgroup_subsys_enabled(memory_cgrp_subsys);
>>> @@ -773,23 +779,26 @@ struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){
>>>     static inline bool obj_cgroup_tryget(struct obj_cgroup *objcg)
>>>   {
>>> +    if (obj_cgroup_is_root(objcg))
>>> +        return true;
>>>       return percpu_ref_tryget(&objcg->refcnt);
>>>   }
>>>   -static inline void obj_cgroup_get(struct obj_cgroup *objcg)
>>> +static inline void obj_cgroup_get_many(struct obj_cgroup *objcg,
>>> +                       unsigned long nr)
>>>   {
>>> -    percpu_ref_get(&objcg->refcnt);
>>> +    if (!obj_cgroup_is_root(objcg))
>>> +        percpu_ref_get_many(&objcg->refcnt, nr);
>>>   }
>>>   -static inline void obj_cgroup_get_many(struct obj_cgroup *objcg,
>>> -                       unsigned long nr)
>>> +static inline void obj_cgroup_get(struct obj_cgroup *objcg)
>>>   {
>>> -    percpu_ref_get_many(&objcg->refcnt, nr);
>>> +    obj_cgroup_get_many(objcg, 1);
>>>   }
>>>     static inline void obj_cgroup_put(struct obj_cgroup *objcg)
>>>   {
>>> -    if (objcg)
>>> +    if (objcg && !obj_cgroup_is_root(objcg))
>>>           percpu_ref_put(&objcg->refcnt);
>>>   }
>>>   @@ -1084,6 +1093,11 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
>>>       return true;
>>>   }
>>>   +static inline bool obj_cgroup_is_root(const struct obj_cgroup *objcg)
>>> +{
>>> +    return true;
>>> +}
>>> +
>>>   static inline bool mem_cgroup_disabled(void)
>>>   {
>>>       return true;
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 544b3200db12d..21b5aad34cae7 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -83,6 +83,8 @@ EXPORT_SYMBOL(memory_cgrp_subsys);
>>>   struct mem_cgroup *root_mem_cgroup __read_mostly;
>>>   EXPORT_SYMBOL(root_mem_cgroup);
>>>   +struct obj_cgroup *root_obj_cgroup __read_mostly;
>>> +
>>>   /* Active memory cgroup to use from an interrupt context */
>>>   DEFINE_PER_CPU(struct mem_cgroup *, int_active_memcg);
>>>   EXPORT_PER_CPU_SYMBOL_GPL(int_active_memcg);
>>> @@ -2634,15 +2636,14 @@ struct mem_cgroup *mem_cgroup_from_slab_obj(void *p)
>>>     static struct obj_cgroup *__get_obj_cgroup_from_memcg(struct mem_cgroup *memcg)
>>>   {
>>> -    struct obj_cgroup *objcg = NULL;
>>> +    for (; memcg; memcg = parent_mem_cgroup(memcg)) {
>>> +        struct obj_cgroup *objcg = rcu_dereference(memcg->objcg);
>>>   -    for (; !mem_cgroup_is_root(memcg); memcg = parent_mem_cgroup(memcg)) {
>>> -        objcg = rcu_dereference(memcg->objcg);
>>>           if (likely(objcg && obj_cgroup_tryget(objcg)))
>>> -            break;
>>> -        objcg = NULL;
>>> +            return objcg;
>>>       }
>>> -    return objcg;
>>> +
>>> +    return NULL;
>>>   }
>>>     static struct obj_cgroup *current_objcg_update(void)
>>> @@ -2716,18 +2717,17 @@ __always_inline struct obj_cgroup *current_obj_cgroup(void)
>>>            * Objcg reference is kept by the task, so it's safe
>>>            * to use the objcg by the current task.
>>>            */
>>> -        return objcg;
>>> +        return objcg ? : root_obj_cgroup;
>>>       }
>>>         memcg = this_cpu_read(int_active_memcg);
>>>       if (unlikely(memcg))
>>>           goto from_memcg;
>>>   -    return NULL;
>>> +    return root_obj_cgroup;
>>>     from_memcg:
>>> -    objcg = NULL;
>>> -    for (; !mem_cgroup_is_root(memcg); memcg = parent_mem_cgroup(memcg)) {
>>> +    for (; memcg; memcg = parent_mem_cgroup(memcg)) {
>>>           /*
>>>            * Memcg pointer is protected by scope (see set_active_memcg())
>>>            * and is pinning the corresponding objcg, so objcg can't go
>>> @@ -2736,10 +2736,10 @@ __always_inline struct obj_cgroup *current_obj_cgroup(void)
>>>            */
>>>           objcg = rcu_dereference_check(memcg->objcg, 1);
>>>           if (likely(objcg))
>>> -            break;
>>> +            return objcg;
>>>       }
>>>   -    return objcg;
>>> +    return root_obj_cgroup;
>>>   }
>>>     struct obj_cgroup *get_obj_cgroup_from_folio(struct folio *folio)
>>> @@ -2753,14 +2753,8 @@ struct obj_cgroup *get_obj_cgroup_from_folio(struct folio *folio)
>>>           objcg = __folio_objcg(folio);
>>>           obj_cgroup_get(objcg);
>>>       } else {
>>> -        struct mem_cgroup *memcg;
>>> -
>>>           rcu_read_lock();
>>> -        memcg = __folio_memcg(folio);
>>> -        if (memcg)
>>> -            objcg = __get_obj_cgroup_from_memcg(memcg);
>>> -        else
>>> -            objcg = NULL;
>>> +        objcg = __get_obj_cgroup_from_memcg(__folio_memcg(folio));
>>>           rcu_read_unlock();
>>>       }
>>>       return objcg;
>>> @@ -2863,7 +2857,7 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
>>>       int ret = 0;
>>>         objcg = current_obj_cgroup();
>>> -    if (objcg) {
>>> +    if (objcg && !obj_cgroup_is_root(objcg)) {
>>>           ret = obj_cgroup_charge_pages(objcg, gfp, 1 << order);
>>>           if (!ret) {
>>>               obj_cgroup_get(objcg);
>>> @@ -3164,7 +3158,7 @@ bool __memcg_slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
>>>        * obj_cgroup_get() is used to get a permanent reference.
>>>        */
>>>       objcg = current_obj_cgroup();
>>> -    if (!objcg)
>>> +    if (!objcg || obj_cgroup_is_root(objcg))
>>>           return true;
>>>         /*
>>> @@ -3851,6 +3845,9 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
>>>       if (!objcg)
>>>           goto free_shrinker;
>>>   +    if (unlikely(mem_cgroup_is_root(memcg)))
>>> +        root_obj_cgroup = objcg;
>>> +
>>>       objcg->memcg = memcg;
>>>       rcu_assign_pointer(memcg->objcg, objcg);
>>>       obj_cgroup_get(objcg);
>>> @@ -5471,6 +5468,9 @@ void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size)
>>>       if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>>>           return;
>>>   +    if (obj_cgroup_is_root(objcg))
>>> +        return;
>>> +
>>>       VM_WARN_ON_ONCE(!(current->flags & PF_MEMALLOC));
>>>         /* PF_MEMALLOC context, charging must succeed */
>>> @@ -5498,6 +5498,9 @@ void obj_cgroup_uncharge_zswap(struct obj_cgroup *objcg, size_t size)
>>>       if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>>>           return;
>>>   +    if (obj_cgroup_is_root(objcg))
>>> +        return;
>>> +
>>>       obj_cgroup_uncharge(objcg, size);
>>>   
>> If we modify zswap by adding MEMCG_ZSWAP_B and MEMCG_ZSWAPPED with obj_cgroup_charge_zswap , then
>> remove a control group (via rmdir) and reparent its objects to the root cgroup, then for the root
>> cgroup, obj_cgroup_uncharge_zswap will return directly due to the obj_cgroup_is_root check. Would
>> this cause us to miss decrementing MEMCG_ZSWAP_B and MEMCG_ZSWAPPED?
> 
> I'm not sure I fully understand the problem—how could this happen, given that
> obj_cgroup_charge_zswap also checks for the root objcg?
> 
> Muchun,
> Thanks.

That is:

1. memcg A is under the root memcg.
2. obj_cgroup_charge_zswap charges memcg A.
3. After rmdir A, the obj of A is reparented to the root memcg.
4. obj_cgroup_uncharge_zswap does nothing and returns, since the object is now associated with the root.

Thus, the root memcg will miss decrementing MEMCG_ZSWAP_B and MEMCG_ZSWAPPED, correct? Or am I
missing something?

>>
>>>       rcu_read_lock();
>>> diff --git a/mm/percpu.c b/mm/percpu.c
>>> index 81462ce5866e1..5c1a9b77d6b93 100644
>>> --- a/mm/percpu.c
>>> +++ b/mm/percpu.c
>>> @@ -1616,7 +1616,7 @@ static bool pcpu_memcg_pre_alloc_hook(size_t size, gfp_t gfp,
>>>           return true;
>>>         objcg = current_obj_cgroup();
>>> -    if (!objcg)
>>> +    if (!objcg || obj_cgroup_is_root(objcg))
>>>           return true;
>>>         if (obj_cgroup_charge(objcg, gfp, pcpu_obj_full_size(size)))
> 

-- 
Best regards,
Ridong