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

[Harry Yoo <harry.yoo@oracle.com>]

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...

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

-- 
Cheers,
Harry / Hyeonggon