Re: [External] Re: [PATCH RFC 26/28] mm: memcontrol: introduce memcg_reparent_ops

[Muchun Song <songmuchun@bytedance.com>]

On Wed, Jul 2, 2025 at 6:13 AM Harry Yoo <harry.yoo@oracle.com> wrote:
>
> On Mon, Jun 30, 2025 at 09:47:25PM +0900, Harry Yoo wrote:
> > On Tue, Apr 15, 2025 at 10:45:30AM +0800, Muchun Song wrote:
> > > In the previous patch, we established a method to ensure the safety of the
> > > lruvec lock and the split queue lock during the reparenting of LRU folios.
> > > The process involves the following steps:
> > >
> > >     memcg_reparent_objcgs(memcg)
> > >         1) lock
> > >         // lruvec belongs to memcg and lruvec_parent belongs to parent memcg.
> > >         spin_lock(&lruvec->lru_lock);
> > >         spin_lock(&lruvec_parent->lru_lock);
> > >
> > >         2) relocate from current memcg to its parent
> > >         // Move all the pages from the lruvec list to the parent lruvec list.
> > >
> > >         3) unlock
> > >         spin_unlock(&lruvec_parent->lru_lock);
> > >         spin_unlock(&lruvec->lru_lock);
> > >
> > > In addition to the folio lruvec lock, the deferred split queue lock
> > > (specific to THP) also requires a similar approach. Therefore, we abstract
> > > the three essential steps from the memcg_reparent_objcgs() function.
> > >
> > >     memcg_reparent_objcgs(memcg)
> > >         1) lock
> > >         memcg_reparent_ops->lock(memcg, parent);
> > >
> > >         2) relocate
> > >         memcg_reparent_ops->relocate(memcg, reparent);
> > >
> > >         3) unlock
> > >         memcg_reparent_ops->unlock(memcg, reparent);
> > >
> > > Currently, two distinct locks (such as the lruvec lock and the deferred
> > > split queue lock) need to utilize this infrastructure. In the subsequent
> > > patch, we will employ these APIs to ensure the safety of these locks
> > > during the reparenting of LRU folios.
> > >
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > ---
> > >  include/linux/memcontrol.h | 20 ++++++++++++
> > >  mm/memcontrol.c            | 62 ++++++++++++++++++++++++++++++--------
> > >  2 files changed, 69 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 27b23e464229..0e450623f8fa 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -311,6 +311,26 @@ struct mem_cgroup {
> > >     struct mem_cgroup_per_node *nodeinfo[];
> > >  };
> > >
> > > +struct memcg_reparent_ops {
> > > +   /*
> > > +    * Note that interrupt is disabled before calling those callbacks,
> > > +    * so the interrupt should remain disabled when leaving those callbacks.
> > > +    */
> > > +   void (*lock)(struct mem_cgroup *src, struct mem_cgroup *dst);
> > > +   void (*relocate)(struct mem_cgroup *src, struct mem_cgroup *dst);
> > > +   void (*unlock)(struct mem_cgroup *src, struct mem_cgroup *dst);
> > > +};
> > > +
> > > +#define DEFINE_MEMCG_REPARENT_OPS(name)                                    \
> > > +   const struct memcg_reparent_ops memcg_##name##_reparent_ops = { \
> > > +           .lock           = name##_reparent_lock,                 \
> > > +           .relocate       = name##_reparent_relocate,             \
> > > +           .unlock         = name##_reparent_unlock,               \
> > > +   }
> > > +
> > > +#define DECLARE_MEMCG_REPARENT_OPS(name)                           \
> > > +   extern const struct memcg_reparent_ops memcg_##name##_reparent_ops
> > > +
> > >  /*
> > >   * size of first charge trial.
> > >   * TODO: maybe necessary to use big numbers in big irons or dynamic based of the
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 1f0c6e7b69cc..3fac51179186 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -194,24 +194,60 @@ static struct obj_cgroup *obj_cgroup_alloc(void)
> > >     return objcg;
> > >  }
> > >
> > > -static void memcg_reparent_objcgs(struct mem_cgroup *memcg)
> > > +static void objcg_reparent_lock(struct mem_cgroup *src, struct mem_cgroup *dst)
> > > +{
> > > +   spin_lock(&objcg_lock);
> > > +}
> > > +
> > > +static void objcg_reparent_relocate(struct mem_cgroup *src, struct mem_cgroup *dst)
> > >  {
> > >     struct obj_cgroup *objcg, *iter;
> > > -   struct mem_cgroup *parent = parent_mem_cgroup(memcg);
> > >
> > > -   objcg = rcu_replace_pointer(memcg->objcg, NULL, true);
> > > +   objcg = rcu_replace_pointer(src->objcg, NULL, true);
> > > +   /* 1) Ready to reparent active objcg. */
> > > +   list_add(&objcg->list, &src->objcg_list);
> > > +   /* 2) Reparent active objcg and already reparented objcgs to dst. */
> > > +   list_for_each_entry(iter, &src->objcg_list, list)
> > > +           WRITE_ONCE(iter->memcg, dst);
> > > +   /* 3) Move already reparented objcgs to the dst's list */
> > > +   list_splice(&src->objcg_list, &dst->objcg_list);
> > > +}
> > >
> > > -   spin_lock_irq(&objcg_lock);
> > > +static void objcg_reparent_unlock(struct mem_cgroup *src, struct mem_cgroup *dst)
> > > +{
> > > +   spin_unlock(&objcg_lock);
> > > +}
> > >
> > > -   /* 1) Ready to reparent active objcg. */
> > > -   list_add(&objcg->list, &memcg->objcg_list);
> > > -   /* 2) Reparent active objcg and already reparented objcgs to parent. */
> > > -   list_for_each_entry(iter, &memcg->objcg_list, list)
> > > -           WRITE_ONCE(iter->memcg, parent);
> > > -   /* 3) Move already reparented objcgs to the parent's list */
> > > -   list_splice(&memcg->objcg_list, &parent->objcg_list);
> > > -
> > > -   spin_unlock_irq(&objcg_lock);
> > > +static DEFINE_MEMCG_REPARENT_OPS(objcg);
> > > +
> > > +static const struct memcg_reparent_ops *memcg_reparent_ops[] = {
> > > +   &memcg_objcg_reparent_ops,
> > > +};
> > > +
> > > +#define DEFINE_MEMCG_REPARENT_FUNC(phase)                          \
> > > +   static void memcg_reparent_##phase(struct mem_cgroup *src,      \
> > > +                                      struct mem_cgroup *dst)      \
> > > +   {                                                               \
> > > +           int i;                                                  \
> > > +                                                                   \
> > > +           for (i = 0; i < ARRAY_SIZE(memcg_reparent_ops); i++)    \
> > > +                   memcg_reparent_ops[i]->phase(src, dst);         \
> > > +   }
> > > +
> > > +DEFINE_MEMCG_REPARENT_FUNC(lock)
> > > +DEFINE_MEMCG_REPARENT_FUNC(relocate)
> > > +DEFINE_MEMCG_REPARENT_FUNC(unlock)
> > > +
> > > +static void memcg_reparent_objcgs(struct mem_cgroup *src)
> > > +{
> > > +   struct mem_cgroup *dst = parent_mem_cgroup(src);
> > > +   struct obj_cgroup *objcg = rcu_dereference_protected(src->objcg, true);
> > > +
> > > +   local_irq_disable();
> > > +   memcg_reparent_lock(src, dst);
> > > +   memcg_reparent_relocate(src, dst);
> > > +   memcg_reparent_unlock(src, dst);
> > > +   local_irq_enable();
> >
> > Hi,
> >
> > It seems unnecessarily complicated to 1) acquire objcg, lruvec and
> > thp_sq locks, 2) call their ->relocate() callbacks, and
> > 3) release those locks.
> >
> > Why not simply do the following instead?
> >
> > for (i = 0; i < ARRAY_SIZE(memcg_reparent_ops); i++) {
> >       local_irq_disable();
> >       memcg_reparent_ops[i]->lock(src, dst);
> >       memcg_reparent_ops[i]->relocate(src, dst);
> >       memcg_reparent_ops[i]->unlock(src, dst);
> >       local_irq_enable();
> > }
> >
> > As there is no actual lock dependency between the three.
> >
> > Or am I missing something important about the locking requirements?
>
> Hmm... looks like I was missing some important requirements!
>
> It seems like:
>
> 1) objcg should be reparented under lruvec locks, otherwise
>    users can observe folio_memcg(folio) != lruvec_memcg(lruvec)
>
> 2) Similarly, lruvec_reparent_relocate() should reparent all folios
>    at once under lruvec locks, otherwise users can observe
>    folio_memcg(folio) != lruvec_memcg(lruvec) for some folios.
>
>    IoW, lruvec_reparent_relocate() cannot do something like this:
>    while (lruvec is not empty) {
>            move some pages;
>            unlock lruvec locks;
>            cond_resched();
>            lock lruvec locks;
>    }
>
> Failing to satisfy 1) and 2) means user can't rely on a stable binding
> between a folio and a memcg, which is a no-go.
>
> Also, 2) makes it quite undesirable to iterate over folios and move each
> one to the right generation in MGLRU as this will certainly introduce
> soft lockups as the memcg size grows...

Sorry for the late reply. Yes, you are right. We should iterate each folio
without holding any spinlocks. So I am confirming if we could do some
preparation work like making it suitable for reparenting without holding any
spinlock. Then we could reparent those folios like the non-MGLRU case.

Thanks.

>
> Is my reasoning correct?
> If so, adding a brief comment about 1 and 2 wouldn't hurt ;)

OK.

>
> --
> Cheers,
> Harry / Hyeonggon