[PATCH 0/2] Fix excessive swapping when memcg are enabled

[PATCH 0/2] Fix excessive swapping when memcg are enabled

[Jerome Marchand]

When memory cgroups are enabled, reclaim code may force scan of
anonymous page in one memcg even when there are plenty of file pages
in other memcg. It has lead to excessive swapping in a real life
example: a virtual machine running in a memcg while there is
background I/O.

The first patch just updates an outdated comment that has bugged me
for a while but that I never bothered to update. The second patch
actually fixes the issue.

Jerome Marchand (2):
  mm, vmscan: fix an outdated comment still mentioning get_scan_ratio
  memcg, vmscan: Fix forced anon scan

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

-- 
1.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] mm, vmscan: fix an outdated comment still mentioning get_scan_ratio

[Jerome Marchand]

Quite a while  ago, get_scan_ratio() has been renamed get_scan_count(),
however a comment in shrink_active_list() still mention it. This patch
fixes the outdated comment.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index d698f4f..079918d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1759,7 +1759,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
 	 * Count referenced pages from currently used mappings as rotated,
 	 * even though only some of them are actually re-activated.  This
 	 * helps balance scan pressure between file and anonymous pages in
-	 * get_scan_ratio.
+	 * get_scan_count.
 	 */
 	reclaim_stat->recent_rotated[file] += nr_rotated;
 
-- 
1.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Jerome Marchand]

When memory cgoups are enabled, the code that decides to force to scan
anonymous pages in get_scan_count() compares global values (free,
high_watermark) to a value that is restricted to a memory cgroup
(file). It make the code over-eager to force anon scan.

For instance, it will force anon scan when scanning a memcg that is
mainly populated by anonymous page, even when there is plenty of file
pages to get rid of in others memcgs, even when swappiness == 0. It
breaks user's expectation about swappiness and hurts performance. 

This patch make sure that forced anon scan only happens when there not
enough file pages for the all zone, not just in one random memcg.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 mm/vmscan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 079918d..3ad2069 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1950,8 +1950,11 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
 	 */
 	if (global_reclaim(sc)) {
 		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
+		unsigned long zonefile =
+			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
+			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
 
-		if (unlikely(file + free <= high_wmark_pages(zone))) {
+		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
 			scan_balance = SCAN_ANON;
 			goto out;
 		}
-- 
1.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Michal Hocko]

On Thu 31-07-14 13:49:45, Jerome Marchand wrote:
> When memory cgoups are enabled, the code that decides to force to scan
> anonymous pages in get_scan_count() compares global values (free,
> high_watermark) to a value that is restricted to a memory cgroup
> (file). It make the code over-eager to force anon scan.

OK, I though this was about memcg reclaim according to the subject but
this is in fact global reclaim when there are multiple memcgs present.
Good. You are right that apples are compared to oranges here.

> For instance, it will force anon scan when scanning a memcg that is
> mainly populated by anonymous page, even when there is plenty of file
> pages to get rid of in others memcgs, even when swappiness == 0. It
> breaks user's expectation about swappiness and hurts performance. 
> 
> This patch make sure that forced anon scan only happens when there not
> enough file pages for the all zone, not just in one random memcg.

OK, makes sense to me.

> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

Although I have never seen this before I can imagine specialized memcgs
running with mostly anon memory so I would even consider it a stable
material.

Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
>  mm/vmscan.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 079918d..3ad2069 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1950,8 +1950,11 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>  	 */
>  	if (global_reclaim(sc)) {
>  		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
> +		unsigned long zonefile =
> +			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
> +			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
>  
> -		if (unlikely(file + free <= high_wmark_pages(zone))) {
> +		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
>  			scan_balance = SCAN_ANON;
>  			goto out;
>  		}

You could move file and anon further down when we actually use them.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Johannes Weiner]

On Thu, Jul 31, 2014 at 01:49:45PM +0200, Jerome Marchand wrote:
> When memory cgoups are enabled, the code that decides to force to scan
> anonymous pages in get_scan_count() compares global values (free,
> high_watermark) to a value that is restricted to a memory cgroup
> (file). It make the code over-eager to force anon scan.
> 
> For instance, it will force anon scan when scanning a memcg that is
> mainly populated by anonymous page, even when there is plenty of file
> pages to get rid of in others memcgs, even when swappiness == 0. It
> breaks user's expectation about swappiness and hurts performance. 
> 
> This patch make sure that forced anon scan only happens when there not
> enough file pages for the all zone, not just in one random memcg.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/31/2014 07:49 AM, Jerome Marchand wrote:
> When memory cgoups are enabled, the code that decides to force to
> scan anonymous pages in get_scan_count() compares global values
> (free, high_watermark) to a value that is restricted to a memory
> cgroup (file). It make the code over-eager to force anon scan.
> 
> For instance, it will force anon scan when scanning a memcg that
> is mainly populated by anonymous page, even when there is plenty of
> file pages to get rid of in others memcgs, even when swappiness ==
> 0. It breaks user's expectation about swappiness and hurts
> performance.
> 
> This patch make sure that forced anon scan only happens when there
> not enough file pages for the all zone, not just in one random
> memcg.
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

That fix is a lot smaller than I thought it would be. Nice.

Reviewed-by: Rik van Riel <riel@redhat.com>

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJT2mL7AAoJEM553pKExN6DbzsH/ArKqWXYFfz7/hjADJXz85aK
ygWdjpK18MbFeUMW3nL324j2567TXWpC2G7SgxSPjYnF/qvKjpoQHJk7WvisymjE
p+5jGQAxzXgjlq0usGoFRrWUnR6vkdjTx0K8r6MO/asMLbvDBjkXvaURHdcV6fx4
nUbkF/GRXGAGcnHOEks294w+8j8R50bugnX+IfmKo73eteNcMWU7Ga+b93kUmz3p
4EE2PRpRKFWtpTAhpFlFI46gfu+e7I1Ziu2pzNUlYOP3P7t9pRS8YOI5JNOnyDfi
lrbOXzoSqs6sbIlDd//A/p7u6Pzr+HnpbaxCrf9UCdNaMMqvb0gDQWv7221gI24=
=BfHz
-----END PGP SIGNATURE-----

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Johannes Weiner]

On Thu, Jul 31, 2014 at 02:30:26PM +0200, Michal Hocko wrote:
> On Thu 31-07-14 13:49:45, Jerome Marchand wrote:
> > @@ -1950,8 +1950,11 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
> >  	 */
> >  	if (global_reclaim(sc)) {
> >  		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
> > +		unsigned long zonefile =
> > +			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
> > +			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
> >  
> > -		if (unlikely(file + free <= high_wmark_pages(zone))) {
> > +		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
> >  			scan_balance = SCAN_ANON;
> >  			goto out;
> >  		}
> 
> You could move file and anon further down when we actually use them.

Agreed with that.  Can we merge this into the original patch?

---

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Michal Hocko]

On Fri 01-08-14 14:45:25, Johannes Weiner wrote:
> On Thu, Jul 31, 2014 at 02:30:26PM +0200, Michal Hocko wrote:
> > On Thu 31-07-14 13:49:45, Jerome Marchand wrote:
> > > @@ -1950,8 +1950,11 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
> > >  	 */
> > >  	if (global_reclaim(sc)) {
> > >  		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
> > > +		unsigned long zonefile =
> > > +			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
> > > +			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
> > >  
> > > -		if (unlikely(file + free <= high_wmark_pages(zone))) {
> > > +		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
> > >  			scan_balance = SCAN_ANON;
> > >  			goto out;
> > >  		}
> > 
> > You could move file and anon further down when we actually use them.
> 
> Agreed with that.  Can we merge this into the original patch?
> 
> ---
> From e49bef8d2751d9b27f1733e3e0eced325ffce700 Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Fri, 1 Aug 2014 10:48:26 -0400
> Subject: [patch] memcg, vmscan: Fix forced scan of anonymous pages fix -
>  cleanups
> 
> o Use enum zone_stat_item symbols directly to select zone stats,
>   rather than NR_LRU_BASE plus LRU index
> 
> o scanned/rotated scaling is the only user of the lruvec anon/file
>   counters, so move the reads of those values to right before that
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Yes, please.
Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!

> ---
>  mm/vmscan.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b3f629bdf4fe..2836b5373b2e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1934,11 +1934,6 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>  		goto out;
>  	}
>  
> -	anon  = get_lru_size(lruvec, LRU_ACTIVE_ANON) +
> -		get_lru_size(lruvec, LRU_INACTIVE_ANON);
> -	file  = get_lru_size(lruvec, LRU_ACTIVE_FILE) +
> -		get_lru_size(lruvec, LRU_INACTIVE_FILE);
> -
>  	/*
>  	 * Prevent the reclaimer from falling into the cache trap: as
>  	 * cache pages start out inactive, every cache fault will tip
> @@ -1949,12 +1944,14 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>  	 * anon pages.  Try to detect this based on file LRU size.
>  	 */
>  	if (global_reclaim(sc)) {
> -		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
> -		unsigned long zonefile =
> -			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
> -			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
> +		unsigned long zonefile;
> +		unsigned long zonefree;
> +
> +		zonefree = zone_page_state(zone, NR_FREE_PAGES);
> +		zonefile = zone_page_state(zone, NR_ACTIVE_FILE) +
> +			   zone_page_state(zone, NR_INACTIVE_FILE);
>  
> -		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
> +		if (unlikely(zonefile + zonefree <= high_wmark_pages(zone))) {
>  			scan_balance = SCAN_ANON;
>  			goto out;
>  		}
> @@ -1989,6 +1986,12 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>  	 *
>  	 * anon in [0], file in [1]
>  	 */
> +
> +	anon  = get_lru_size(lruvec, LRU_ACTIVE_ANON) +
> +		get_lru_size(lruvec, LRU_INACTIVE_ANON);
> +	file  = get_lru_size(lruvec, LRU_ACTIVE_FILE) +
> +		get_lru_size(lruvec, LRU_INACTIVE_FILE);
> +
>  	spin_lock_irq(&zone->lru_lock);
>  	if (unlikely(reclaim_stat->recent_scanned[0] > anon / 4)) {
>  		reclaim_stat->recent_scanned[0] /= 2;
> -- 
> 2.0.3
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] memcg, vmscan: Fix forced scan of anonymous pages

[Jerome Marchand]

[-- Attachment #1: Type: text/plain, Size: 3860 bytes --]

On 08/01/2014 08:52 PM, Michal Hocko wrote:
> On Fri 01-08-14 14:45:25, Johannes Weiner wrote:
>> On Thu, Jul 31, 2014 at 02:30:26PM +0200, Michal Hocko wrote:
>>> On Thu 31-07-14 13:49:45, Jerome Marchand wrote:
>>>> @@ -1950,8 +1950,11 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>>>>  	 */
>>>>  	if (global_reclaim(sc)) {
>>>>  		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
>>>> +		unsigned long zonefile =
>>>> +			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
>>>> +			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
>>>>  
>>>> -		if (unlikely(file + free <= high_wmark_pages(zone))) {
>>>> +		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
>>>>  			scan_balance = SCAN_ANON;
>>>>  			goto out;
>>>>  		}
>>>
>>> You could move file and anon further down when we actually use them.

I missed that comment. Thanks for the cleanup Johannes!

>>
>> Agreed with that.  Can we merge this into the original patch?
>>
>> ---
>> From e49bef8d2751d9b27f1733e3e0eced325ffce700 Mon Sep 17 00:00:00 2001
>> From: Johannes Weiner <hannes@cmpxchg.org>
>> Date: Fri, 1 Aug 2014 10:48:26 -0400
>> Subject: [patch] memcg, vmscan: Fix forced scan of anonymous pages fix -
>>  cleanups
>>
>> o Use enum zone_stat_item symbols directly to select zone stats,
>>   rather than NR_LRU_BASE plus LRU index
>>
>> o scanned/rotated scaling is the only user of the lruvec anon/file
>>   counters, so move the reads of those values to right before that
>>
>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Yes, please.
> Acked-by: Michal Hocko <mhocko@suse.cz>

Acked-by: Jerome Marchand <jmarchan@redhat.com>

> 
> Thanks!
> 
>> ---
>>  mm/vmscan.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index b3f629bdf4fe..2836b5373b2e 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1934,11 +1934,6 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>>  		goto out;
>>  	}
>>  
>> -	anon  = get_lru_size(lruvec, LRU_ACTIVE_ANON) +
>> -		get_lru_size(lruvec, LRU_INACTIVE_ANON);
>> -	file  = get_lru_size(lruvec, LRU_ACTIVE_FILE) +
>> -		get_lru_size(lruvec, LRU_INACTIVE_FILE);
>> -
>>  	/*
>>  	 * Prevent the reclaimer from falling into the cache trap: as
>>  	 * cache pages start out inactive, every cache fault will tip
>> @@ -1949,12 +1944,14 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>>  	 * anon pages.  Try to detect this based on file LRU size.
>>  	 */
>>  	if (global_reclaim(sc)) {
>> -		unsigned long free = zone_page_state(zone, NR_FREE_PAGES);
>> -		unsigned long zonefile =
>> -			zone_page_state(zone, NR_LRU_BASE + LRU_ACTIVE_FILE) +
>> -			zone_page_state(zone, NR_LRU_BASE + LRU_INACTIVE_FILE);
>> +		unsigned long zonefile;
>> +		unsigned long zonefree;
>> +
>> +		zonefree = zone_page_state(zone, NR_FREE_PAGES);
>> +		zonefile = zone_page_state(zone, NR_ACTIVE_FILE) +
>> +			   zone_page_state(zone, NR_INACTIVE_FILE);
>>  
>> -		if (unlikely(zonefile + free <= high_wmark_pages(zone))) {
>> +		if (unlikely(zonefile + zonefree <= high_wmark_pages(zone))) {
>>  			scan_balance = SCAN_ANON;
>>  			goto out;
>>  		}
>> @@ -1989,6 +1986,12 @@ static void get_scan_count(struct lruvec *lruvec, int swappiness,
>>  	 *
>>  	 * anon in [0], file in [1]
>>  	 */
>> +
>> +	anon  = get_lru_size(lruvec, LRU_ACTIVE_ANON) +
>> +		get_lru_size(lruvec, LRU_INACTIVE_ANON);
>> +	file  = get_lru_size(lruvec, LRU_ACTIVE_FILE) +
>> +		get_lru_size(lruvec, LRU_INACTIVE_FILE);
>> +
>>  	spin_lock_irq(&zone->lru_lock);
>>  	if (unlikely(reclaim_stat->recent_scanned[0] > anon / 4)) {
>>  		reclaim_stat->recent_scanned[0] /= 2;
>> -- 
>> 2.0.3
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 538 bytes --]