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

[Michal Hocko]

On Wed 12-12-12 17:28:44, Johannes Weiner wrote:
> 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... :-)

Yes, I think that the result would be worse (more swapping) so the
change can only help.

> 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.

Yes the patch makes sense for memcg as well. I guess you have tested
this primarily with memcg. Do you have any numbers? Would be nice to put
them into the changelog if you have (it should help to reduce swapping
with heavy streaming IO load).

Acked-by: Michal Hocko <mhocko@xxxxxxx>
-- 
Michal Hocko
SUSE Labs
--
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/