Re: [PATCH 2/4] mm: thp: introduce folio_split_queue_lock and its variants

[Qi Zheng]

Hi Zi,

On 9/19/25 11:39 PM, Zi Yan wrote:
> On 18 Sep 2025, at 23:46, Qi Zheng wrote:
>
> > From: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> >
> > In future memcg removal, the binding between a folio and a memcg may
> > change, making the split lock within the memcg unstable when held.
> >
> > A new approach is required to reparent the split queue to its parent. This
> > patch starts introducing a unified way to acquire the split lock for
> > future work.
> >
> > It's a code-only refactoring with no functional changes.
> >
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > ---
> >   include/linux/memcontrol.h | 10 +++++
> >   mm/huge_memory.c           | 89 ++++++++++++++++++++++++++------------
> >   2 files changed, 71 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 16fe0306e50ea..99876af13c315 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -1662,6 +1662,11 @@ int alloc_shrinker_info(struct mem_cgroup *memcg);
> >   void free_shrinker_info(struct mem_cgroup *memcg);
> >   void set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id);
> >   void reparent_shrinker_deferred(struct mem_cgroup *memcg);
> > +
> > +static inline int shrinker_id(struct shrinker *shrinker)
> > +{
> > +	return shrinker->id;
> > +}
> >   #else
> >   #define mem_cgroup_sockets_enabled 0
> >
> > @@ -1693,6 +1698,11 @@ static inline void set_shrinker_bit(struct mem_cgroup *memcg,
> >   				    int nid, int shrinker_id)
> >   {
> >   }
> > +
> > +static inline int shrinker_id(struct shrinker *shrinker)
> > +{
> > +	return -1;
> > +}
> >   #endif
> >
> >   #ifdef CONFIG_MEMCG
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 582628ddf3f33..d34516a22f5bb 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1078,26 +1078,62 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
> >
> >   #ifdef CONFIG_MEMCG
> >   static inline
> > -struct deferred_split *get_deferred_split_queue(struct folio *folio)
> > +struct mem_cgroup *folio_split_queue_memcg(struct folio *folio,
> > +					   struct deferred_split *queue)
> >   {
> > -	struct mem_cgroup *memcg = folio_memcg(folio);
> > -	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
> > -
> > -	if (memcg)
> > -		return &memcg->deferred_split_queue;
> > -	else
> > -		return &pgdat->deferred_split_queue;
> > +	if (mem_cgroup_disabled())
> > +		return NULL;
> > +	if (&NODE_DATA(folio_nid(folio))->deferred_split_queue == queue)
> > +		return NULL;
> > +	return container_of(queue, struct mem_cgroup, deferred_split_queue);
> >   }
> >   #else
> >   static inline
> > -struct deferred_split *get_deferred_split_queue(struct folio *folio)
> > +struct mem_cgroup *folio_split_queue_memcg(struct folio *folio,
> > +					   struct deferred_split *queue)
> >   {
> > -	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
> > -
> > -	return &pgdat->deferred_split_queue;
> > +	return NULL;
> >   }
> >   #endif
> >
> > +static struct deferred_split *folio_split_queue_lock(struct folio *folio)
> > +{
> > +	struct mem_cgroup *memcg;
> > +	struct deferred_split *queue;
> > +
> > +	memcg = folio_memcg(folio);
> > +	queue = memcg ? &memcg->deferred_split_queue :
> > +			&NODE_DATA(folio_nid(folio))->deferred_split_queue;
> > +	spin_lock(&queue->split_queue_lock);
> > +
> > +	return queue;
> > +}
> > +
> > +static struct deferred_split *
> > +folio_split_queue_lock_irqsave(struct folio *folio, unsigned long *flags)
> > +{
> > +	struct mem_cgroup *memcg;
> > +	struct deferred_split *queue;
> > +
> > +	memcg = folio_memcg(folio);
> > +	queue = memcg ? &memcg->deferred_split_queue :
> > +			&NODE_DATA(folio_nid(folio))->deferred_split_queue;
> > +	spin_lock_irqsave(&queue->split_queue_lock, *flags);
> > +
> > +	return queue;
> > +}
>
> A helper function to get queue from a folio would get rid of duplicated
> code in the two functions above. Hmm, that is the deleted
> get_deferred_split_queue(). So probably retain it.

After PATCH #4, we may retry after getting parent memcg in the above
functions. so we may not need to retrieve get_deferred_split_queue().

> Otherwise, LGTM. Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

Thanks!

> Best Regards,
> Yan, Zi