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

[Bingfang Guo]

Forwarding review from Sashiko:
> > 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)
> >  {
> >  	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);
> 
> Does returning the nearest online ancestor instead of the originally recorded
> dying memcg cause issues for callers that need to uncharge the exact memcg
> that was charged?

This seems to be a false positive?

The page counters are charged recursively to the root. When we get some ancestor
of the dying (or dead) memcg, the dead ones will never be accessed again so just
uncharging the online parents should be enough? And these counters are also left
untouched when the memcg is offline and pages are reparented to the ancestors
together with the objcg.  So I think this should be fine.

> > +
> > +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);
> > +	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);
> 
> When a swap entry is charged while a memcg is online, the original memcg's
> local MEMCG_SWAP per-cpu stat is incremented and its swap page_counter is
> charged.
> If this memcg is subsequently offlined, it becomes dying. When the swap is
> eventually freed, mem_cgroup_private_id_put() now returns the nearest
> online ancestor instead of the original dying memcg.
> Will applying the uncharge to this ancestor permanently leak the counter in
> the original dying memcg?
> Furthermore, does this erroneously decrement the ancestor's local MEMCG_SWAP
> stat, causing a permanent stat underflow since the ancestor did not receive
> the corresponding local charge?
> 

We are holding rcu read lock and checked !memcg_is_dying(memcg) in
mem_cgroup_private_id_put(). When the css is being offlined, CSS_DYING is first
set and then mem_cgroup_css_offline() is called behind call_rcu, so no
concurrent offlining of this memcg can take place here.

For cgroup v1, MEMCG_SWAP stats will be reparented so eventually we will get
consistent result. And for cgroup v2, the stat is not visible (and therefore not
reparented) so no problem as well.

> > -		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)
> > 
> > -- 
> > 2.43.7
> > 
> >