Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633918 > unrolled thread

[patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is  low David Rientjes <rientjes@google.com> - 2017-05-01 23:40 +0200
    Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low Michal Hocko <mhocko@kernel.org> - 2017-05-02 10:10 +0200
      Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low David Rientjes <rientjes@google.com> - 2017-05-02 22:50 +0200
        Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low Michal Hocko <mhocko@kernel.org> - 2017-05-03 08:20 +0200
          Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low Michal Hocko <mhocko@kernel.org> - 2017-05-03 09:10 +0200
            Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low Michal Hocko <mhocko@kernel.org> - 2017-05-03 11:00 +0200
              Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low David Rientjes <rientjes@google.com> - 2017-05-04 01:00 +0200
                Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file  is low Michal Hocko <mhocko@kernel.org> - 2017-05-04 13:50 +0200

#1633918 — [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

The purpose of the code that commit 623762517e23 ("revert 'mm: vmscan: do
not swap anon pages just because free+file is low'") reintroduces is to
prefer swapping anonymous memory rather than trashing the file lru.

If the anonymous inactive lru for the set of eligible zones is considered
low, however, or the length of the list for the given reclaim priority
does not allow for effective anonymous-only reclaiming, then avoid
forcing SCAN_ANON.  Forcing SCAN_ANON will end up thrashing the small
list and leave unreclaimed memory on the file lrus.

If the inactive list is insufficient, fallback to balanced reclaim so the
file lru doesn't remain untouched.

Suggested-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 to akpm: this issue has been possible since at least 3.15, so it's
 probably not high priority for 4.12 but applies cleanly if it can sneak
 in

 mm/vmscan.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2204,8 +2204,17 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 		}
 
 		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
-			scan_balance = SCAN_ANON;
-			goto out;
+			/*
+			 * Force SCAN_ANON if there are enough inactive
+			 * anonymous pages on the LRU in eligible zones.
+			 * Otherwise, the small LRU gets thrashed.
+			 */
+			if (!inactive_list_is_low(lruvec, false, sc, false) &&
+			    lruvec_lru_size(lruvec, LRU_INACTIVE_ANON, sc->reclaim_idx)
+					>> sc->priority) {
+				scan_balance = SCAN_ANON;
+				goto out;
+			}
 		}
 	}
 

[toc] | [next] | [standalone]

#1634216 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Mon 01-05-17 14:34:21, David Rientjes wrote:
[...]
> @@ -2204,8 +2204,17 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  		}
>  
>  		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
> -			scan_balance = SCAN_ANON;
> -			goto out;
> +			/*
> +			 * Force SCAN_ANON if there are enough inactive
> +			 * anonymous pages on the LRU in eligible zones.
> +			 * Otherwise, the small LRU gets thrashed.
> +			 */
> +			if (!inactive_list_is_low(lruvec, false, sc, false) &&
> +			    lruvec_lru_size(lruvec, LRU_INACTIVE_ANON, sc->reclaim_idx)
> +					>> sc->priority) {
> +				scan_balance = SCAN_ANON;
> +				goto out;
> +			}

I have already asked and my questions were ignored. So let me ask again
and hopefuly not get ignored this time. So Why do we need a different
criterion on anon pages than file pages? I do agree that blindly
scanning anon pages when file pages are low is very suboptimal but this
adds yet another heuristic without _any_ numbers. Why cannot we simply
treat anon and file pages equally? Something like the following

	if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
		scan_balance = SCAN_FILE;
		if (pgdatfile < pgdatanon)
			scan_balance = SCAN_ANON;
		goto out;
	}

Also it would help to describe the workload which can trigger this
behavior so that we can compare numbers before and after this patch.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1634616 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Tue, 2 May 2017, Michal Hocko wrote:

> I have already asked and my questions were ignored. So let me ask again
> and hopefuly not get ignored this time. So Why do we need a different
> criterion on anon pages than file pages?

The preference in get_scan_count() as already implemented is to reclaim 
from file pages if there is enough memory on the inactive list to reclaim.  
That is unchanged with this patch.

> I do agree that blindly
> scanning anon pages when file pages are low is very suboptimal but this
> adds yet another heuristic without _any_ numbers. Why cannot we simply
> treat anon and file pages equally? Something like the following
> 
> 	if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
> 		scan_balance = SCAN_FILE;
> 		if (pgdatfile < pgdatanon)
> 			scan_balance = SCAN_ANON;
> 		goto out;
> 	}
> 

This would be substantially worse than the current code because it 
thrashes the anon lru when anon out numbers file pages rather than at the 
point we fall under the high watermarks for all eligible zones.  If you 
tested your suggestion, you could see gigabytes of memory left untouched 
on the file lru.  Anonymous memory is more probable to be part of the 
working set.

> Also it would help to describe the workload which can trigger this
> behavior so that we can compare numbers before and after this patch.

Any workload that fills system RAM with anonymous memory that cannot be 
reclaimed will thrash the anon lru without this patch.

[toc] | [prev] | [next] | [standalone]

#1634766 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Tue 02-05-17 13:41:23, David Rientjes wrote:
> On Tue, 2 May 2017, Michal Hocko wrote:
> 
> > I have already asked and my questions were ignored. So let me ask again
> > and hopefuly not get ignored this time. So Why do we need a different
> > criterion on anon pages than file pages?
> 
> The preference in get_scan_count() as already implemented is to reclaim 
> from file pages if there is enough memory on the inactive list to reclaim.  
> That is unchanged with this patch.

My fault, I was too vague. My question was basically why should we use
a different criterion to SCAN_ANON than SCAN_FILE.

> > I do agree that blindly
> > scanning anon pages when file pages are low is very suboptimal but this
> > adds yet another heuristic without _any_ numbers. Why cannot we simply
> > treat anon and file pages equally? Something like the following
> > 
> > 	if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
> > 		scan_balance = SCAN_FILE;
> > 		if (pgdatfile < pgdatanon)
> > 			scan_balance = SCAN_ANON;
> > 		goto out;
> > 	}
> > 
> 
> This would be substantially worse than the current code because it 
> thrashes the anon lru when anon out numbers file pages rather than at the 
> point we fall under the high watermarks for all eligible zones.  If you 
> tested your suggestion, you could see gigabytes of memory left untouched 
> on the file lru.  Anonymous memory is more probable to be part of the 
> working set.

This was supposed to be more an example of a direction I was thinking,
definitely not a final patch. I will think more to come up with a
more complete proposal.

> > Also it would help to describe the workload which can trigger this
> > behavior so that we can compare numbers before and after this patch.
> 
> Any workload that fills system RAM with anonymous memory that cannot be 
> reclaimed will thrash the anon lru without this patch.

I have already asked, but I do not understand why this anon memory
couldn't be reclaimed. Who is pinning it? Why cannot it be swapped out?
If it is mlocked it should be moved to unevictable LRU. What am I
missing?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1634777 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Wed 03-05-17 08:15:28, Michal Hocko wrote:
> On Tue 02-05-17 13:41:23, David Rientjes wrote:
> > On Tue, 2 May 2017, Michal Hocko wrote:
[...]
> > > I do agree that blindly
> > > scanning anon pages when file pages are low is very suboptimal but this
> > > adds yet another heuristic without _any_ numbers. Why cannot we simply
> > > treat anon and file pages equally? Something like the following
> > > 
> > > 	if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
> > > 		scan_balance = SCAN_FILE;
> > > 		if (pgdatfile < pgdatanon)
> > > 			scan_balance = SCAN_ANON;
> > > 		goto out;
> > > 	}
> > > 
> > 
> > This would be substantially worse than the current code because it 
> > thrashes the anon lru when anon out numbers file pages rather than at the 
> > point we fall under the high watermarks for all eligible zones.  If you 
> > tested your suggestion, you could see gigabytes of memory left untouched 
> > on the file lru.  Anonymous memory is more probable to be part of the 
> > working set.
> 
> This was supposed to be more an example of a direction I was thinking,
> definitely not a final patch. I will think more to come up with a
> more complete proposal.

This is still untested but should be much closer to what I've had in
mind.
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 24efcc20af91..bcdad30f942d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2174,8 +2174,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 		}
 
 		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
-			scan_balance = SCAN_ANON;
-			goto out;
+			unsigned long pgdatanon;
+
+			pgdatanon = node_page_state(pgdat, NR_ACTIVE_ANON) +
+				node_page_state(pgdat, NR_INACTIVE_ANON);
+			if (pgdatanon + pgdatfree > total_high_wmark) {
+				scan_balance = SCAN_ANON;
+				goto out;
+			}
 		}
 	}
 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1634816 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Wed 03-05-17 09:06:56, Michal Hocko wrote:
[...]
> This is still untested but should be much closer to what I've had in
> mind.
> ---
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 24efcc20af91..bcdad30f942d 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2174,8 +2174,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  		}
>  
>  		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
> -			scan_balance = SCAN_ANON;
> -			goto out;
> +			unsigned long pgdatanon;
> +
> +			pgdatanon = node_page_state(pgdat, NR_ACTIVE_ANON) +
> +				node_page_state(pgdat, NR_INACTIVE_ANON);
> +			if (pgdatanon + pgdatfree > total_high_wmark) {
> +				scan_balance = SCAN_ANON;
> +				goto out;
> +			}
>  		}
>  	}

I've realized that this just makes the situation more obscure than
necessary after thinking some more about it. It also doesn't achieve my
original intention to treat biased anon and file LRUs the same. Now that
I've digested the change more thoroughly I am willing to ack your patch.
So feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
And sorry about the diversion here but I am always nervous when touching
g_s_c because this tends to lead to subtle issues.

Maybe we could make this aspect of the biased LRUs more explicit by
doing the following rather than duplicating the condition. What do you
think?
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 24efcc20af91..f3ec8760dc06 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2113,16 +2113,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	u64 denominator = 0;	/* gcc */
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 	unsigned long anon_prio, file_prio;
-	enum scan_balance scan_balance;
+	enum scan_balance scan_balance = SCAN_FILE;
 	unsigned long anon, file;
 	unsigned long ap, fp;
 	enum lru_list lru;
 
 	/* If we have no swap space, do not bother scanning anon pages. */
-	if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0) {
-		scan_balance = SCAN_FILE;
+	if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0)
 		goto out;
-	}
 
 	/*
 	 * Global reclaim will swap to prevent OOM even with no
@@ -2131,10 +2129,8 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	 * using the memory controller's swap limit feature would be
 	 * too expensive.
 	 */
-	if (!global_reclaim(sc) && !swappiness) {
-		scan_balance = SCAN_FILE;
+	if (!global_reclaim(sc) && !swappiness)
 		goto out;
-	}
 
 	/*
 	 * Do not apply any pressure balancing cleverness when the
@@ -2147,8 +2143,9 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 	}
 
 	/*
-	 * Prevent the reclaimer from falling into the cache trap: as
-	 * cache pages start out inactive, every cache fault will tip
+	 * We usually want to bias page cache reclaim over anonymous
+	 * memory. Prevent the reclaimer from falling into the cache trap:
+	 * as cache pages start out inactive, every cache fault will tip
 	 * the scan balance towards the file LRU.  And as the file LRU
 	 * shrinks, so does the window for rotation from references.
 	 * This means we have a runaway feedback loop where a tiny
@@ -2173,26 +2170,24 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 			total_high_wmark += high_wmark_pages(zone);
 		}
 
-		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
+		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark))
 			scan_balance = SCAN_ANON;
-			goto out;
-		}
 	}
 
 	/*
-	 * If there is enough inactive page cache, i.e. if the size of the
-	 * inactive list is greater than that of the active list *and* the
-	 * inactive list actually has some pages to scan on this priority, we
-	 * do not reclaim anything from the anonymous working set right now.
-	 * Without the second condition we could end up never scanning an
-	 * lruvec even if it has plenty of old anonymous pages unless the
-	 * system is under heavy pressure.
+	 * Make sure there are enough pages on the biased LRU before we go
+	 * and do an exclusive reclaim from that list, i.e. if the
+	 * size of the inactive list is greater than that of the active list
+	 * *and* the inactive list actually has some pages to scan on this
+	 * priority.
+	 * Without the second condition we could end up never scanning other
+	 * lruvecs even if they have plenty of old pages unless the system is
+	 * under heavy pressure.
 	 */
-	if (!inactive_list_is_low(lruvec, true, memcg, sc, false) &&
-	    lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, sc->reclaim_idx) >> sc->priority) {
-		scan_balance = SCAN_FILE;
+	lru = LRU_INACTIVE_ANON + LRU_FILE * (scan_balance == SCAN_FILE);
+	if (!inactive_list_is_low(lruvec, is_file_lru(lru), memcg, sc, false) &&
+	    lruvec_lru_size(lruvec, lru, sc->reclaim_idx) >> sc->priority)
 		goto out;
-	}
 
 	scan_balance = SCAN_FRACT;
 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1635327 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Wed, 3 May 2017, Michal Hocko wrote:

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 24efcc20af91..f3ec8760dc06 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2113,16 +2113,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  	u64 denominator = 0;	/* gcc */
>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	unsigned long anon_prio, file_prio;
> -	enum scan_balance scan_balance;
> +	enum scan_balance scan_balance = SCAN_FILE;
>  	unsigned long anon, file;
>  	unsigned long ap, fp;
>  	enum lru_list lru;
>  
>  	/* If we have no swap space, do not bother scanning anon pages. */
> -	if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0) {
> -		scan_balance = SCAN_FILE;
> +	if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Global reclaim will swap to prevent OOM even with no
> @@ -2131,10 +2129,8 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  	 * using the memory controller's swap limit feature would be
>  	 * too expensive.
>  	 */
> -	if (!global_reclaim(sc) && !swappiness) {
> -		scan_balance = SCAN_FILE;
> +	if (!global_reclaim(sc) && !swappiness)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Do not apply any pressure balancing cleverness when the

Good as a cleanup so far.

> @@ -2147,8 +2143,9 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  	}
>  
>  	/*
> -	 * Prevent the reclaimer from falling into the cache trap: as
> -	 * cache pages start out inactive, every cache fault will tip
> +	 * We usually want to bias page cache reclaim over anonymous
> +	 * memory. Prevent the reclaimer from falling into the cache trap:
> +	 * as cache pages start out inactive, every cache fault will tip
>  	 * the scan balance towards the file LRU.  And as the file LRU
>  	 * shrinks, so does the window for rotation from references.
>  	 * This means we have a runaway feedback loop where a tiny

I think Minchan made a good point earlier about anon being more likely to 
be working set since it is mapped, but this may be a biased opinion coming 
from me since I am primarily concerned with malloc.

> @@ -2173,26 +2170,24 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
>  			total_high_wmark += high_wmark_pages(zone);
>  		}
>  
> -		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
> +		if (unlikely(pgdatfile + pgdatfree <= total_high_wmark))
>  			scan_balance = SCAN_ANON;
> -			goto out;
> -		}
>  	}
>  
>  	/*
> -	 * If there is enough inactive page cache, i.e. if the size of the
> -	 * inactive list is greater than that of the active list *and* the
> -	 * inactive list actually has some pages to scan on this priority, we
> -	 * do not reclaim anything from the anonymous working set right now.
> -	 * Without the second condition we could end up never scanning an
> -	 * lruvec even if it has plenty of old anonymous pages unless the
> -	 * system is under heavy pressure.
> +	 * Make sure there are enough pages on the biased LRU before we go
> +	 * and do an exclusive reclaim from that list, i.e. if the
> +	 * size of the inactive list is greater than that of the active list
> +	 * *and* the inactive list actually has some pages to scan on this
> +	 * priority.
> +	 * Without the second condition we could end up never scanning other
> +	 * lruvecs even if they have plenty of old pages unless the system is
> +	 * under heavy pressure.
>  	 */
> -	if (!inactive_list_is_low(lruvec, true, memcg, sc, false) &&
> -	    lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, sc->reclaim_idx) >> sc->priority) {
> -		scan_balance = SCAN_FILE;
> +	lru = LRU_INACTIVE_ANON + LRU_FILE * (scan_balance == SCAN_FILE);

This part seems to complicate the logic since it determines the lru under 
test based on the current setting of scan_balance.  I think I prefer 
individual heuristics with well written comments, but others may feel 
differently about this.

> +	if (!inactive_list_is_low(lruvec, is_file_lru(lru), memcg, sc, false) &&
> +	    lruvec_lru_size(lruvec, lru, sc->reclaim_idx) >> sc->priority)
>  		goto out;
> -	}
>  
>  	scan_balance = SCAN_FRACT;
>  

[toc] | [prev] | [next] | [standalone]

#1635653 — Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low

On Wed 03-05-17 15:52:04, David Rientjes wrote:
> On Wed, 3 May 2017, Michal Hocko wrote:
[...]
> >  	/*
> > -	 * If there is enough inactive page cache, i.e. if the size of the
> > -	 * inactive list is greater than that of the active list *and* the
> > -	 * inactive list actually has some pages to scan on this priority, we
> > -	 * do not reclaim anything from the anonymous working set right now.
> > -	 * Without the second condition we could end up never scanning an
> > -	 * lruvec even if it has plenty of old anonymous pages unless the
> > -	 * system is under heavy pressure.
> > +	 * Make sure there are enough pages on the biased LRU before we go
> > +	 * and do an exclusive reclaim from that list, i.e. if the
> > +	 * size of the inactive list is greater than that of the active list
> > +	 * *and* the inactive list actually has some pages to scan on this
> > +	 * priority.
> > +	 * Without the second condition we could end up never scanning other
> > +	 * lruvecs even if they have plenty of old pages unless the system is
> > +	 * under heavy pressure.
> >  	 */
> > -	if (!inactive_list_is_low(lruvec, true, memcg, sc, false) &&
> > -	    lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, sc->reclaim_idx) >> sc->priority) {
> > -		scan_balance = SCAN_FILE;
> > +	lru = LRU_INACTIVE_ANON + LRU_FILE * (scan_balance == SCAN_FILE);
> 
> This part seems to complicate the logic since it determines the lru under 
> test based on the current setting of scan_balance.  I think I prefer 
> individual heuristics with well written comments, but others may feel 
> differently about this.

I do not claim the code would more obvious than before but it gets rid
of the duplication which is usually a good thing. This size check has
the same reasoning regardless of the type of the LRU. But I am not going
to insist...
 
> > +	if (!inactive_list_is_low(lruvec, is_file_lru(lru), memcg, sc, false) &&
> > +	    lruvec_lru_size(lruvec, lru, sc->reclaim_idx) >> sc->priority)
> >  		goto out;
> > -	}
> >  
> >  	scan_balance = SCAN_FRACT;
> >  

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web