Re: [PATCH RFC v2 3/6] mm/memcg: pass the id itself instead of memcg for putting ID

[Bingfang Guo]

On Sat, Sep 05, 2026 at 06:01:44PM +0800, Muchun Song wrote:
> 
> 
> On 2026/9/1 16:58, Bingfang Guo via B4 Relay wrote:
>> From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>> 
>> Swap uncharge knows the memcg only by its private id, and the id can
>> outlive the memcg it used to belong to after we rebind memcgid to
>> objcgs.  Make mem_cgroup_private_id_put() take the id and resolve the
>> memcg containing the refcount internally, and keep the underlying
>> __mem_cgroup_private_id_put() for the offline path that still holds a
>> memcg pointer.
>> 
>> In the uncharge path, the memcg pointer will have to be read from the
>> xarray twice, but we'll fix that later by returning the memcg from the
>> put path, so the uncharge path can obtain a reference in the same step.
>> 
>> Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>> ---
>> mm/memcontrol.c | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index ecb4fb07d7735..048c9bb0fad79 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4038,7 +4038,7 @@ static void mem_cgroup_private_id_remove(struct mem_cgroup *memcg)
>> 	}
>> }
>> -static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned int n)
>> +static void __mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned int n)
>> {
>> 	if (refcount_sub_and_test(n, &memcg->id.ref)) {
>> 		mem_cgroup_private_id_remove(memcg);
>> @@ -4048,9 +4048,19 @@ static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsigned
>> 	}
>> }
>> +static void mem_cgroup_private_id_put(unsigned short id, unsigned int n)
>> +{
>> +	struct mem_cgroup *memcg;
>> +
>> +	rcu_read_lock();
>> +	memcg = mem_cgroup_from_private_id(id);
>> +	__mem_cgroup_private_id_put(memcg, n);
>> +	rcu_read_unlock();
>> +}
> 
> To me, the direction of this change makes sense. At the same
> time, I would also suggest applying a similar change to the get
> function, for example: change mem_cgroup_private_id_get_online
> to:
> 
>    unsigned short mem_cgroup_private_id_get(struct mem_cgroup *memcg,
>                                             unsigned int n);
> 
> Have it return a private ID. That way, I think the two interfaces
> look much more symmetrical overall.
> 
> Based on this change, it should be easier to implement the
> direction of my last suggestion in Patch 5.
> 
> Muchun,
> Thanks.
> 

After taking a closer look at Patch 2, I find that charging the
counter before getting memcgid is problematic if the current
memcg is dying.

If the memcg is not root, the swap counter will be charged. But
if it is also dying, we'll get the root's ID and store that in
the swap table, which creates an inconsistent state and leak the
charge when we do swap uncharge, find that the corresponding
objcg is root and skip uncharging the counter.

On the other hand, if we restore the old logic to get an ID first
and then charge if the ID refers to a root objcg, we'll have to
access the xarray again to retrieve the objcg, or return the
objcg using a pointer in the parameter list. And both are not
ideal to me.

I think a better plan is to just check the root status of the
memcgid itself.  We can store the root memcg's ID just as
root_mem_cgroup and add a function:

  bool mem_cgroup_private_id_is_root(unsigned short id);

This makes memcgid related operations a family: we have get, put
and can check its root status independently, without having to go
through another object.  And we can use it to decide whether we
should skip root charge/uncharge for all three functions
(memcgv1, swap charge and uncharge).

>> +
>> static void mem_cgroup_private_id_kill(struct mem_cgroup *memcg)
>> {
>> -	mem_cgroup_private_id_put(memcg, 1);
>> +	__mem_cgroup_private_id_put(memcg, 1);
>> }
>> struct mem_cgroup *mem_cgroup_private_id_get_online(struct mem_cgroup *memcg, unsigned int n)
>> @@ -5815,9 +5825,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(memcg, nr_pages);
>> +		mem_cgroup_private_id_put(id, nr_pages);
>> 	}
>> 	rcu_read_unlock();
>> +
>> }
>> long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
>> 
>