[PATCHv3] mm: remove '!root_reclaim' checking in should_abort_scan()

[PATCHv3] mm: remove '!root_reclaim' checking in should_abort_scan()

[zhaoyang.huang]

From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

Android systems usually use memory.reclaim interface to implement user
space memory management which expects that the requested reclaim target
and actually reclaimed amount memory are not diverging by too much. With
the current MGRLU implementation there is, however, no bail out when the
reclaim target is reached and this could lead to an excessive reclaim
that scales with the reclaim hierarchy size.For example, we can get a
nr_reclaimed=394/nr_to_reclaim=32 proactive reclaim under a common 1-N
cgroup hierarchy.
This defect arised from the goal of keeping fairness among memcgs that
is, for try_to_free_mem_cgroup_pages -> shrink_node_memcgs ->
shrink_lruvec -> lru_gen_shrink_lruvec -> try_to_shrink_lruvec, the
!root_reclaim(sc) check was there for reclaim fairness, which was
necessary before commit 'b82b530740b9' ("mm: vmscan: restore
incremental cgroup iteration") because the fairness depended on
attempted proportional reclaim from every memcg under the target
memcg. However after commit 'b82b530740b9' there is no longer a need
to visit every memcg to ensure fairness. Let's have try_to_shrink_lruvec
bail out when the nr_reclaimed achieved.

Suggested-by: T.J.Mercier <tjmercier@google.com>
Reviewed-by: T.J.Mercier <tjmercier@google.com>
Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
Patchv2,v3: update commit message
---
---
 mm/vmscan.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 0fc9373e8251..10f1e7d716ca 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -4839,10 +4839,6 @@ static bool should_abort_scan(struct lruvec *lruvec, struct scan_control *sc)
 	int i;
 	enum zone_watermarks mark;
 
-	/* don't abort memcg reclaim to ensure fairness */
-	if (!root_reclaim(sc))
-		return false;
-
 	if (sc->nr_reclaimed >= max(sc->nr_to_reclaim, compact_gap(sc->order)))
 		return true;
 
-- 
2.25.1

Re: [PATCHv3] mm: remove '!root_reclaim' checking in should_abort_scan()

[Michal Hocko]

On Wed 18-03-26 09:15:58, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Android systems usually use memory.reclaim interface to implement user
> space memory management which expects that the requested reclaim target
> and actually reclaimed amount memory are not diverging by too much. With
> the current MGRLU implementation there is, however, no bail out when the
> reclaim target is reached and this could lead to an excessive reclaim
> that scales with the reclaim hierarchy size.For example, we can get a
> nr_reclaimed=394/nr_to_reclaim=32 proactive reclaim under a common 1-N
> cgroup hierarchy.
> This defect arised from the goal of keeping fairness among memcgs that
> is, for try_to_free_mem_cgroup_pages -> shrink_node_memcgs ->
> shrink_lruvec -> lru_gen_shrink_lruvec -> try_to_shrink_lruvec, the
> !root_reclaim(sc) check was there for reclaim fairness, which was
> necessary before commit 'b82b530740b9' ("mm: vmscan: restore
> incremental cgroup iteration") because the fairness depended on
> attempted proportional reclaim from every memcg under the target
> memcg. However after commit 'b82b530740b9' there is no longer a need
> to visit every memcg to ensure fairness. Let's have try_to_shrink_lruvec
> bail out when the nr_reclaimed achieved.
> 
> Suggested-by: T.J.Mercier <tjmercier@google.com>
> Reviewed-by: T.J.Mercier <tjmercier@google.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

OK, this is easier to understand. Although you have avoided some
clarifications I have asked for explicitly in prior version. If MGLRU
maintainers are OK with this then just go ahead. Btw. I have only now
noticed that they have not been involved. Adding them now.

> ---
> Patchv2,v3: update commit message
> ---
> ---
>  mm/vmscan.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 0fc9373e8251..10f1e7d716ca 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4839,10 +4839,6 @@ static bool should_abort_scan(struct lruvec *lruvec, struct scan_control *sc)
>  	int i;
>  	enum zone_watermarks mark;
>  
> -	/* don't abort memcg reclaim to ensure fairness */
> -	if (!root_reclaim(sc))
> -		return false;
> -
>  	if (sc->nr_reclaimed >= max(sc->nr_to_reclaim, compact_gap(sc->order)))
>  		return true;
>  
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs