Re: [PATCH RFC v2 6/6] mm/memcg: filter out reparented memcgs got from memcgid

[Bingfang Guo]

Forwarding reviews from Sashiko:
>> From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>> 
>> mem_cgroup_from_private_id() looks up the objcg that owns the id and
>> returns the objcg's current memcg.  After reparenting, that memcg can
>> differ from the one the id originally belonged to.
>> 
>> Callers such as the list lru and workingset refault code expect to get
>> back exactly the memcg referred to by the memcgid, so check that the
>> returned memcg still owns the id and return NULL otherwise. In this
>> case, make list_lru just skip and workingset tests use the root (as the
>> comments said in workingset.c:460).
>> 
>> Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>> ---
>> mm/list_lru.c   | 2 +-
>> mm/memcontrol.c | 9 ++++++++-
>> mm/workingset.c | 2 +-
>> 3 files changed, 10 insertions(+), 3 deletions(-)
>> 
>> diff --git a/mm/list_lru.c b/mm/list_lru.c
>> index 36662d02ff963..bc956267f6835 100644
>> --- a/mm/list_lru.c
>> +++ b/mm/list_lru.c
>> @@ -428,7 +428,7 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
>> 		xa_for_each(&lru->xa, index, mlru) {
>> 			rcu_read_lock();
>> 			memcg = mem_cgroup_from_private_id(index);
>> -			if (!mem_cgroup_tryget(memcg)) {
>> +			if (!memcg || !mem_cgroup_tryget(memcg)) {
>> 				rcu_read_unlock();
>> 				continue;
>> 			}
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 38d2b00657a7a..84dcab3acb8a2 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4112,17 +4112,24 @@ struct mem_cgroup *mem_cgroup_private_id_get_online(struct mem_cgroup *memcg, un
>>  * @id: the memcg id to look up
>>  *
>>  * Caller must hold rcu_read_lock().
>> + *
>> + * @return: the memcg, or NULL if the memcg is already reparented.
>>  */
>> struct mem_cgroup *mem_cgroup_from_private_id(unsigned short id)
>> {
>> 	struct obj_cgroup *objcg;
>> +	struct mem_cgroup *memcg;
>> 	WARN_ON_ONCE(!rcu_read_lock_held());
>> 
>> 	objcg = xa_load(&mem_cgroup_private_ids, id);
>> 	if (!objcg)
>> 		return NULL;
>> 
>> -	return obj_cgroup_memcg(objcg);
>> +	memcg = obj_cgroup_memcg(objcg);
>> +	if (mem_cgroup_private_id(memcg) != id)
>> +		return NULL;
>> +
>> +	return memcg;
>> }
> 
> Does returning NULL here for reparented memcgs break swapin accounting
> in callers like mem_cgroup_swapin_charge_folio()?
> If an offlined memcg is reparented, its original ID no longer matches the
> inheritor's ID. When mem_cgroup_swapin_charge_folio() calls this function:
> 	memcg = mem_cgroup_from_private_id(id);
> 	if (!memcg || !css_tryget_online(&memcg->css))
> 		memcg = get_mem_cgroup_from_mm(mm);
> Would receiving NULL force it to wrongfully charge the faulting task instead
> of the parent memcg that inherited the swap charge, creating a permanent
> accounting mismatch?
> 

The original behavior uses css_tryget_online(), which filters out offlined
memcgs as well so I suppose no behavioral change here.

And swap (or memsw) counter of the memcg from which the page was swapped out
should be uncharged in __mem_cgroup_uncharge_swap() when the swap entry is
removed.

I will put this in the commit message to explain it later.

>> 
>> struct mem_cgroup *mem_cgroup_get_from_id(u64 id)
>> diff --git a/mm/workingset.c b/mm/workingset.c
>> index 7ac2b88c80ae5..4e9b7b163c01e 100644
>> --- a/mm/workingset.c
>> +++ b/mm/workingset.c
>> @@ -470,7 +470,7 @@ bool workingset_test_recent(void *shadow, bool file, bool *workingset,
>> 	 * configurations instead.
>> 	 */
>> 	eviction_memcg = mem_cgroup_from_private_id(memcgid);
>> -	if (!mem_cgroup_tryget(eviction_memcg))
>> +	if (eviction_memcg && !mem_cgroup_tryget(eviction_memcg))
>> 		eviction_memcg = NULL;
>> 	rcu_read_unlock();
> 
> The commit message states that workingset tests use the root (as the
> comments said in workingset.c:460) when NULL is returned. However, is
> this what actually happens?
> Looking immediately below this diff in workingset_test_recent(), it seems
> receiving NULL triggers an early return:
> 	if (!mem_cgroup_disabled() && !eviction_memcg)
> 		return false;
> Does this abort the refault calculation entirely, treating the reparented
> shadow entry as cold and causing page cache thrashing?

I didn't make it clear in the commit message here.  For MGLRU the root is used.
But for classical LRU, it's just skipped because refault reading of other memcgs
should be meaningless in this case.

I will update the commit message to match the behavior...

> Additionally, can this cause an issue in MGLRU lru_gen_test_recent()?
> If mem_cgroup_from_private_id() returns NULL there:
> 	memcg = mem_cgroup_from_private_id(memcg_id);
> 	*lruvec = mem_cgroup_lruvec(memcg, pgdat);
> Would this silently fall back to the root memcg LRU vector instead of the
> correct parent memcg, incorrectly evaluating the child eviction token
> against the root completely unrelated max_seq?
> 

Yes, but the activation is speculative as documented so it should be okay? Are
there better ways to handle this?

But I do think it may be good to keep the two methods behaves consistently, to
either skip the check or get a random result for both two cases.

>> 
>> 
>> -- 
>> 2.43.7
>> 
>>