Re: [PATCH RFC v2 4/6] mm/memcg: return the memcg when putting memcgid

[Muchun Song]

> On Sep 6, 2026, at 03:36, Bingfang Guo <bfguo@xxxxxxxxxx> wrote:
> 
> On Sat, Sep 05, 2026 at 03:28:50PM +0800, Muchun Song wrote:
>> 
> 
> Hi, Muchun!
> 
>> 
>> On 2026/9/1 16:58, Bingfang Guo via B4 Relay wrote:
>>> From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>>> 
>>> __mem_cgroup_uncharge_swap() needs both the memcg and the id refcount
>>> drop.  Right now it looks the memcg up by id, uncharges it, then looks
>>> it up again inside mem_cgroup_private_id_put() to drop the reference.
>>> 
>>> Make mem_cgroup_private_id_put() resolve the id once, drop the
>>> reference, and return the nearest online memcg with a reference held for
>>> the caller.  __mem_cgroup_uncharge_swap() then uses that memcg directly
>>> and drops the reference after uncharging, avoiding the second xarray
>>> lookup.
>>> 
>>> Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>>> ---
>>>  mm/memcontrol.c | 20 +++++++++++++++++---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 048c9bb0fad79..f0503a1e5492d 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -4048,14 +4048,28 @@ static void __mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned int n
>>>   }
>>>  }
>>> -static void mem_cgroup_private_id_put(unsigned short id, unsigned int n)
>>> +/**
>>> + * mem_cgroup_private_id_put - put memcgid and get the nearest online memcg
>>> + * @id: the memcg private id got from mem_cgroup_id_get_online
>>> + * @n: count of references to put
>>> + */
>>> +static struct mem_cgroup *mem_cgroup_private_id_put(unsigned short id, unsigned int n)
>> 
>> Having an API that put reference-counted resources return a struct pointer
>> is a very strange design. Please don't do that.
> 
> That's true.
> 
>> 
>>>  {
>>>   struct mem_cgroup *memcg;
>>>   rcu_read_lock();
>>>   memcg = mem_cgroup_from_private_id(id);
>>> + if (!memcg)
>>> + goto out;
>>> +
>>>   __mem_cgroup_private_id_put(memcg, n);
>>> +
>>> + while (memcg_is_dying(memcg) || !mem_cgroup_tryget(memcg))
>>> + memcg = parent_mem_cgroup(memcg);
>>> +
>>> +out:
>>>   rcu_read_unlock();
>>> + return memcg;
>>>  }
>>>  static void mem_cgroup_private_id_kill(struct mem_cgroup *memcg)
>>> @@ -5816,7 +5830,7 @@ void __mem_cgroup_uncharge_swap(unsigned short id, unsigned int nr_pages)
>>>   struct mem_cgroup *memcg;
>>>   rcu_read_lock();
>>> - memcg = mem_cgroup_from_private_id(id);
>> 
>> I think we can introduce a new helper like obj_cgroup_from_private_id(),
>> We can use the ID to get the corresponding obj_cgroup, and then get the
>> mem_cgroup.
> 
> I actually did this in some local versions but gave up in the end
> because the ID should be referring to some memcg and I was not
> sure if getting some objcg from a memcgid looks normal.

But the current series is already changing the binding
relationship between IDs and pointers. You've already
established a binding between the ID and the objcg, rather
than with the memcg. Therefore, I think this is a very
natural transition.

Muchun,
Thanks.

> 
> So if that is acceptable, I think it's a great idea to do like
> that! And the put can look less weird then.
> 
> Thanks for your idea!
> 
> Regards,
> Bingfang
> 
>> 
>> Muhcun,
>> Thanks.
>>> + memcg = mem_cgroup_private_id_put(id, nr_pages);
>>>   if (memcg) {
>>>   if (!mem_cgroup_is_root(memcg)) {
>>>   if (do_memsw_account())
>>> @@ -5825,10 +5839,10 @@ void __mem_cgroup_uncharge_swap(unsigned short id, unsigned int nr_pages)
>>>   page_counter_uncharge(&memcg->swap, nr_pages);
>>>   }
>>>   mod_memcg_state(memcg, MEMCG_SWAP, -nr_pages);
>>> - mem_cgroup_private_id_put(id, nr_pages);
>>>   }
>>>   rcu_read_unlock();
>>> + mem_cgroup_put(memcg);
>>>  }
>>>  long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)