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

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

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?

-- 
Cheers,
Harry / Hyeonggon

>  
>  	percpu_ref_kill(&objcg->refcnt);
>  }
> -- 
> 2.20.1
>