Re: Re: [PATCH RFC 10/28] mm: memcontrol: return root object cgroup for root memory cgroup

[Muchun Song <songmuchun@bytedance.com>]

On Sat, Jun 28, 2025 at 11:09 AM Chen Ridong <chenridong@huaweicloud.com> wrote:
>
>
>
> On 2025/4/15 10:45, Muchun Song wrote:
> > 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>
> > ---
> >  include/linux/memcontrol.h | 29 ++++++++++++++++++-------
> >  mm/memcontrol.c            | 44 ++++++++++++++++++++------------------
> >  mm/percpu.c                |  2 +-
> >  3 files changed, 45 insertions(+), 30 deletions(-)
> >
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index bb4f203733f3..e74922d5755d 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -319,6 +319,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 */
> > @@ -528,6 +529,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);
> > @@ -752,23 +758,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);
> >  }
> >
> > @@ -1101,6 +1110,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;
> > @@ -1684,8 +1698,7 @@ static inline struct obj_cgroup *get_obj_cgroup_from_current(void)
> >  {
> >       struct obj_cgroup *objcg = current_obj_cgroup();
> >
> > -     if (objcg)
> > -             obj_cgroup_get(objcg);
> > +     obj_cgroup_get(objcg);
> >
> >       return objcg;
> >  }
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index a6362d11b46c..4aadc1b87db3 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -81,6 +81,7 @@ struct cgroup_subsys memory_cgrp_subsys __read_mostly;
> >  EXPORT_SYMBOL(memory_cgrp_subsys);
> >
> >  struct mem_cgroup *root_mem_cgroup __read_mostly;
> > +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);
> > @@ -2525,15 +2526,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;
> >  }
> >
>
> It appears that the return NULL statement might be dead code in this
> context. And would it be preferable to use return root_obj_cgroup instead?

I do not think so. The parameter of @memcg could be NULL passed from
current_objcg_update(). Returning NULL in this case makes sense to me.
It is not reasonable to return root_obj_cgroup for a NULL memcg for me.

Muchun,
Thanks.

>
> Best regards,
> Ridong
>
> >  static struct obj_cgroup *current_objcg_update(void)
> > @@ -2604,18 +2604,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
> > @@ -2624,10 +2623,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)
> > @@ -2641,14 +2640,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;
> > @@ -2733,7 +2726,7 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
> >       int ret = 0;
> >
> >       objcg = current_obj_cgroup();
> > -     if (objcg) {
> > +     if (!obj_cgroup_is_root(objcg)) {
> >               ret = obj_cgroup_charge_pages(objcg, gfp, 1 << order);
> >               if (!ret) {
> >                       obj_cgroup_get(objcg);
> > @@ -3036,7 +3029,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 (obj_cgroup_is_root(objcg))
> >               return true;
> >
> >       /*
> > @@ -3708,6 +3701,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);
> > @@ -5302,6 +5298,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 */
> > @@ -5329,6 +5328,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);
> >
> >       rcu_read_lock();
> > diff --git a/mm/percpu.c b/mm/percpu.c
> > index b35494c8ede2..3e54c6fca9bd 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 (obj_cgroup_is_root(objcg))
> >               return true;
> >
> >       if (obj_cgroup_charge(objcg, gfp, pcpu_obj_full_size(size)))
>