[PATCH] mm: vmscan: Obey indeed proportional scanning for kswapd and memcg

[PATCH] mm: vmscan: Obey indeed proportional scanning for kswapd and memcg

[Yaowei Bai]

Commit e82e0561dae9f3ae5 ("mm: vmscan: obey proportional scanning
requirements for kswapd") intended to preserve the proportional scanning
and reclaim what was requested by get_scan_count() for kswapd and memcg
by stopping reclaiming one type(anon or file) LRU and reducing the other's
amount of scanning proportional to the original scan target.

So the way to determine which LRU should be stopped reclaiming should be
comparing scanned/unscanned percentages to the original scan target of two
lru types instead of absolute values what implemented currently, because
larger absolute value doesn't mean larger percentage, there shall be
chance that larger absolute value with smaller percentage, for instance:

	target_file = 1000
	target_anon = 500
	nr_file = 500
	nr_anon = 400

in this case, because nr_file > nr_anon, according to current implement,
we will stop scanning anon lru and shrink file lru. This breaks
proportional scanning intent and makes more unproportional.

This patch changes to compare percentage to the original scan target to
determine which lru should be shrunk.

Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
---
 mm/vmscan.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 2aec424..09a37436 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2216,6 +2216,7 @@ static void shrink_lruvec(struct lruvec *lruvec, int swappiness,
 	while (nr[LRU_INACTIVE_ANON] || nr[LRU_ACTIVE_FILE] ||
 					nr[LRU_INACTIVE_FILE]) {
 		unsigned long nr_anon, nr_file, percentage;
+		unsigned long percentage_anon, percentage_file;
 		unsigned long nr_scanned;
 
 		for_each_evictable_lru(lru) {
@@ -2250,16 +2251,17 @@ static void shrink_lruvec(struct lruvec *lruvec, int swappiness,
 		if (!nr_file || !nr_anon)
 			break;
 
-		if (nr_file > nr_anon) {
-			unsigned long scan_target = targets[LRU_INACTIVE_ANON] +
-						targets[LRU_ACTIVE_ANON] + 1;
+		percentage_anon = nr_anon * 100 / (targets[LRU_INACTIVE_ANON] +
+						targets[LRU_ACTIVE_ANON] + 1);
+		percentage_file = nr_file * 100 / (targets[LRU_INACTIVE_FILE] +
+						targets[LRU_ACTIVE_FILE] + 1);
+
+		if (percentage_file > percentage_anon) {
 			lru = LRU_BASE;
-			percentage = nr_anon * 100 / scan_target;
+			percentage = percentage_anon;
 		} else {
-			unsigned long scan_target = targets[LRU_INACTIVE_FILE] +
-						targets[LRU_ACTIVE_FILE] + 1;
 			lru = LRU_FILE;
-			percentage = nr_file * 100 / scan_target;
+			percentage = percentage_file;
 		}
 
 		/* Stop scanning the smaller of the LRU */
-- 
1.9.1



--
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] mm: vmscan: Obey indeed proportional scanning for kswapd and memcg

[Mel Gorman]

On Wed, Nov 25, 2015 at 12:48:20PM +0800, Yaowei Bai wrote:
> Commit e82e0561dae9f3ae5 ("mm: vmscan: obey proportional scanning
> requirements for kswapd") intended to preserve the proportional scanning
> and reclaim what was requested by get_scan_count() for kswapd and memcg
> by stopping reclaiming one type(anon or file) LRU and reducing the other's
> amount of scanning proportional to the original scan target.
> 
> So the way to determine which LRU should be stopped reclaiming should be
> comparing scanned/unscanned percentages to the original scan target of two
> lru types instead of absolute values what implemented currently, because
> larger absolute value doesn't mean larger percentage, there shall be
> chance that larger absolute value with smaller percentage, for instance:
> 
> 	target_file = 1000
> 	target_anon = 500
> 	nr_file = 500
> 	nr_anon = 400
> 
> in this case, because nr_file > nr_anon, according to current implement,
> we will stop scanning anon lru and shrink file lru. This breaks
> proportional scanning intent and makes more unproportional.
> 
> This patch changes to compare percentage to the original scan target to
> determine which lru should be shrunk.
> 
> Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>

This one has gone back and forth a few times in the past. It really was
deliberate that the scanning was proportional to the scan target. While
I see what your concern is, it's unclear what the actual impact is. Have
you done any testing to check if the proposed new behaviour is actually
better?

-- 
Mel Gorman
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] mm: vmscan: Obey indeed proportional scanning for kswapd and memcg

[Yaowei Bai]

On Wed, Nov 25, 2015 at 11:28:51AM +0000, Mel Gorman wrote:
> On Wed, Nov 25, 2015 at 12:48:20PM +0800, Yaowei Bai wrote:
> > Commit e82e0561dae9f3ae5 ("mm: vmscan: obey proportional scanning
> > requirements for kswapd") intended to preserve the proportional scanning
> > and reclaim what was requested by get_scan_count() for kswapd and memcg
> > by stopping reclaiming one type(anon or file) LRU and reducing the other's
> > amount of scanning proportional to the original scan target.
> > 
> > So the way to determine which LRU should be stopped reclaiming should be
> > comparing scanned/unscanned percentages to the original scan target of two
> > lru types instead of absolute values what implemented currently, because
> > larger absolute value doesn't mean larger percentage, there shall be
> > chance that larger absolute value with smaller percentage, for instance:
> > 
> > 	target_file = 1000
> > 	target_anon = 500
> > 	nr_file = 500
> > 	nr_anon = 400
> > 
> > in this case, because nr_file > nr_anon, according to current implement,
> > we will stop scanning anon lru and shrink file lru. This breaks
> > proportional scanning intent and makes more unproportional.
> > 
> > This patch changes to compare percentage to the original scan target to
> > determine which lru should be shrunk.
> > 
> > Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> 
> This one has gone back and forth a few times in the past. It really was

Sorry for reply late. Yes, I noticed Johannes Weiner has recommended this in
the discussion thread about commit e82e0561dae9f3ae5 ("mm: vmscan: obey
proportional scanning requirements for kswapd"):

 http://marc.info/?l=linux-kernel&m=136397130117394&w=2

and you thought it was out of scope of that series at that moment.
But i didn't see this in the upstream git history.

> deliberate that the scanning was proportional to the scan target. While

Yes, i see the evolvement of the source code and do believe that the scanning
was proportional to the scan target is the right direction and we're already in
that direction with current implementation. At the very beginning, you wanted
to subtract min from all of LRUs to perform proportional scan and i think that
is a very good start and simple and useful enough approxiamtion. And then Johannes
Weiner suggersted that swappiness is about page types and comparing the sum of
file pages with the sum of anon pages and then knocking out the smaller pair would
be better.You agreed and implemented it with applying scanned percentage of the
smaller pair to the remaining LRUs. But considering the example case mentioned
above we will scan even more unproportionally as we cann't guarantee scanning
all LRUs 100% evenly.

> I see what your concern is, it's unclear what the actual impact is. Have
> you done any testing to check if the proposed new behaviour is actually
> better?

I didn't test this patch. Maybe it's difficult to catch this situation of
the example case because mostly we scan LRUs evenly. but i think it's advantage
is also obvious because it covers the case mentioned above to achieve indeed
proportional without introducing extra overhead and makes the code match with
the comments and more understandable to reduce people's confusion.

Did i miss something?

> 
> -- 
> Mel Gorman
> 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] mm: vmscan: Obey indeed proportional scanning for kswapd and memcg

[Mel Gorman]

On Mon, Nov 30, 2015 at 05:04:44PM +0800, Yaowei Bai wrote:
> On Wed, Nov 25, 2015 at 11:28:51AM +0000, Mel Gorman wrote:
> > On Wed, Nov 25, 2015 at 12:48:20PM +0800, Yaowei Bai wrote:
> > > Commit e82e0561dae9f3ae5 ("mm: vmscan: obey proportional scanning
> > > requirements for kswapd") intended to preserve the proportional scanning
> > > and reclaim what was requested by get_scan_count() for kswapd and memcg
> > > by stopping reclaiming one type(anon or file) LRU and reducing the other's
> > > amount of scanning proportional to the original scan target.
> > > 
> > > So the way to determine which LRU should be stopped reclaiming should be
> > > comparing scanned/unscanned percentages to the original scan target of two
> > > lru types instead of absolute values what implemented currently, because
> > > larger absolute value doesn't mean larger percentage, there shall be
> > > chance that larger absolute value with smaller percentage, for instance:
> > > 
> > > 	target_file = 1000
> > > 	target_anon = 500
> > > 	nr_file = 500
> > > 	nr_anon = 400
> > > 
> > > in this case, because nr_file > nr_anon, according to current implement,
> > > we will stop scanning anon lru and shrink file lru. This breaks
> > > proportional scanning intent and makes more unproportional.
> > > 
> > > This patch changes to compare percentage to the original scan target to
> > > determine which lru should be shrunk.
> > > 
> > > Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> > 
> > This one has gone back and forth a few times in the past. It really was
> 
> Sorry for reply late. Yes, I noticed Johannes Weiner has recommended this in
> the discussion thread about commit e82e0561dae9f3ae5 ("mm: vmscan: obey
> proportional scanning requirements for kswapd"):
> 
>  http://marc.info/?l=linux-kernel&m=136397130117394&w=2
> 
> and you thought it was out of scope of that series at that moment.
> But i didn't see this in the upstream git history.
> 

It was out of scope for the series at the time. The idea is still
interesting but it really needs to be quantified in some manner.

> > <SNIP>
> >
> > I see what your concern is, it's unclear what the actual impact is. Have
> > you done any testing to check if the proposed new behaviour is actually
> > better?
> 
> I didn't test this patch. Maybe it's difficult to catch this situation of
> the example case because mostly we scan LRUs evenly. but i think it's advantage
> is also obvious because it covers the case mentioned above to achieve indeed
> proportional without introducing extra overhead and makes the code match with
> the comments and more understandable to reduce people's confusion.
> 
> Did i miss something?
> 

It really needs to be tested and have some sort of supporting data
showing that it at least does no harm and ideally helps something
worthwhile.

-- 
Mel Gorman
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>