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

[Michal Hocko <mhocko@suse.com>]

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>

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.

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