Re: [Patch v2 1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node()

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Mar 14, 2022 at 10:51:51AM +0100, Michal Hocko wrote:
>On Sat 12-03-22 07:16:21, Wei Yang wrote:
>> When mz is not NULL, mem_cgroup_largest_soft_limit_node() has removed
>> it from rb_tree.
>> 
>> Not necessary to call __mem_cgroup_remove_exceeded() again.
>
>Yes, the call seems to be unnecessary with the current code. mz can
>either come from mem_cgroup_largest_soft_limit_node or
>__mem_cgroup_largest_soft_limit_node both rely on the latter so the mz
>is always off the tree indeed.
> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>After the changelog is completed you can add
>Acked-by: Michal Hocko <mhocko@suse.com>

Will adjust it.

>
>In general, though, I am not a super fan of changes like these. The code
>works as expected, the call for __mem_cgroup_remove_exceeded will not
>really add much of an overhead and at least we can see that mz is always
>removed before it is re-added back. In a hot path I would care much more
>of course but this is effectivelly a dead code as the soft limit itself
>is mostly a relict of past.
>
>Please keep this in mind when you want to make further changes to this
>area. The review is not free of cost and I am not sure spending time on
>this area is worthwhile unless there is a real usecase in mind.
>

Yes, after more understanding of the code, I found soft reclaim seems to be
not that often.

Thanks for your time and will choose some more important area for change.

>Thanks!
>
>> ---
>>  mm/memcontrol.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index f898320b678a..d70bf5cf04eb 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3458,7 +3458,6 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>>  		nr_reclaimed += reclaimed;
>>  		*total_scanned += nr_scanned;
>>  		spin_lock_irq(&mctz->lock);
>> -		__mem_cgroup_remove_exceeded(mz, mctz);
>>  
>>  		/*
>>  		 * If we failed to reclaim anything from this memory cgroup
>> -- 
>> 2.33.1
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me