Re: [patch 1/8] mm: memcg: only evict file pages when we have plenty

[Johannes Weiner]

On Wed, Dec 12, 2012 at 04:53:36PM -0500, Rik van Riel wrote:
> On 12/12/2012 04:43 PM, Johannes Weiner wrote:
> >dc0422c "mm: vmscan: only evict file pages when we have plenty" makes
> >a point of not going for anonymous memory while there is still enough
> >inactive cache around.
> >
> >The check was added only for global reclaim, but it is just as useful
> >for memory cgroup reclaim.
> >
> >Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> >---
> >  mm/vmscan.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index 157bb11..3874dcb 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -1671,6 +1671,16 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> >  		denominator = 1;
> >  		goto out;
> >  	}
> >+	/*
> >+	 * There is enough inactive page cache, do not reclaim
> >+	 * anything from the anonymous working set right now.
> >+	 */
> >+	if (!inactive_file_is_low(lruvec)) {
> >+		fraction[0] = 0;
> >+		fraction[1] = 1;
> >+		denominator = 1;
> >+		goto out;
> >+	}
> >
> >  	anon  = get_lru_size(lruvec, LRU_ACTIVE_ANON) +
> >  		get_lru_size(lruvec, LRU_INACTIVE_ANON);
> >@@ -1688,15 +1698,6 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> >  			fraction[1] = 0;
> >  			denominator = 1;
> >  			goto out;
> >-		} else if (!inactive_file_is_low_global(zone)) {
> >-			/*
> >-			 * There is enough inactive page cache, do not
> >-			 * reclaim anything from the working set right now.
> >-			 */
> >-			fraction[0] = 0;
> >-			fraction[1] = 1;
> >-			denominator = 1;
> >-			goto out;
> >  		}
> >  	}
> >
> >
> 
> I believe the if() block should be moved to AFTER
> the check where we make sure we actually have enough
> file pages.

You are absolutely right, this makes more sense.  Although I'd figure
the impact would be small because if there actually is that little
file cache, it won't be there for long with force-file scanning... :-)

I moved the condition, but it throws conflicts in the rest of the
series.  Will re-run tests, wait for Michal and Mel, then resend.

Thanks, Rik!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/