Re: [PATCH RFC v2 2/6] mm/memcg: get memcgid reference only after swap charging success

[Bingfang Guo]

Forwarding reviews from Sashiko:
> > From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
> > 
> > __mem_cgroup_try_charge_swap() pinned the memcg private id before the
> > swap counter was charged and had to undo the pin on the failure path.
> > Hold RCU lock for an extended period (which should be fine,
> > __memcg1_swapout() does this as well) so concurrent memcg release can
> > be avoided, and take the id reference to its online parent only after
> > charging has succeeded.
> > 
> > The failure path is now a plain return, and the id is only pinned for
> > entries that actually end up charged to swap.
> > 
> > Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
> > ---
> >  mm/memcontrol.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 31cec9dde55f0..ecb4fb07d7735 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5755,6 +5755,7 @@ int __mem_cgroup_try_charge_swap(struct folio *folio)
> >  	struct page_counter *counter;
> >  	struct mem_cgroup *memcg;
> >  	struct obj_cgroup *objcg;
> > +	unsigned short memcgid;
> >  
> >  	if (do_memsw_account())
> >  		return 0;
> > @@ -5772,22 +5773,24 @@ int __mem_cgroup_try_charge_swap(struct folio *folio)
> >  		return 0;
> >  	}
> >  
> > -	memcg = mem_cgroup_private_id_get_online(memcg, nr_pages);
> > -	/* memcg is pined by memcg ID. */
> > -	rcu_read_unlock();
> > +	while (memcg_is_dying(memcg))
> > +		memcg = parent_mem_cgroup(memcg);
> >  
> >  	if (!mem_cgroup_is_root(memcg) &&
> >  	    !page_counter_try_charge(&memcg->swap, nr_pages, &counter)) {
> >  		memcg_memory_event(memcg, MEMCG_SWAP_MAX);
> >  		memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> > -		mem_cgroup_private_id_put(memcg, nr_pages);
> > +		rcu_read_unlock();
> >  		return -ENOMEM;
> >  	}
> >  	mod_memcg_state(memcg, MEMCG_SWAP, nr_pages);
> >  
> > +	memcg = mem_cgroup_private_id_get_online(memcg, nr_pages);
> > +	memcgid = mem_cgroup_private_id(memcg);
> > +	rcu_read_unlock();
> 
> Does this introduce a Time-of-Check to Time-of-Use (TOCTOU) race?
> If a concurrent cgroup rmdir (triggering css_offline) occurs exactly between
> page_counter_try_charge() and mem_cgroup_private_id_get_online(), could it
> drop the child's ID refcount to zero?
> The subsequent ID lookup would climb to the parent, recording the parent's ID
> in the swap cache while the child's counter was already charged. This could
> leave the child's swap page_counter and MEMCG_SWAP stat permanently charged
> when destroyed.
> Additionally, could the parent cgroup's MEMCG_SWAP stat receive a spurious
> uncharge upon swap-in, causing it to permanently under-report swap usage and
> corrupting the memory.stat output?
> 

Just as I replied in patch 4, offlining shouldn't happen with rcu read lock
held, so actually this won't happen and we should always get the memcg itself...
And even if we are getting the ancestor it should be fine, uncharging can be
done for its online ancestor and stat will be consistent.

Maybe we can release the rcu lock earlier or remove the retry logic when we get
the memcgid reference?

> > +
> >  	ci = swap_cluster_get_and_lock(folio);
> > -	__swap_cgroup_set(ci, swp_cluster_offset(folio->swap), nr_pages,
> > -			  mem_cgroup_private_id(memcg));
> > +	__swap_cgroup_set(ci, swp_cluster_offset(folio->swap), nr_pages, memcgid);
> >  	swap_cluster_unlock(ci);
> >  
> >  	return 0;
> > 
> > -- 
> > 2.43.7
> > 
> >