[PATCH] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[PATCH] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[ming.ling]

Non-lru pages don't belong to any lru, so accounting them to
NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
It may misguide functions such as pgdat_reclaimable_pages and
too_many_isolated.

This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
And with non-lru pages in vmstat, it helps to optimize algorithm of
function too_many_isolated oneday.

Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
---
 include/linux/mmzone.h |  1 +
 mm/compaction.c        | 12 +++++++++---
 mm/migrate.c           | 14 ++++++++++----
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 7f2ae99..dc0adba 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -169,6 +169,7 @@ enum node_stat_item {
 	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
 	NR_DIRTIED,		/* page dirtyings since bootup */
 	NR_WRITTEN,		/* page writings since bootup */
+	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
 	NR_VM_NODE_STAT_ITEMS
 };
 
diff --git a/mm/compaction.c b/mm/compaction.c
index 9affb29..8da1dca 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
 static void acct_isolated(struct zone *zone, struct compact_control *cc)
 {
 	struct page *page;
-	unsigned int count[2] = { 0, };
+	unsigned int count[3] = { 0, };
 
 	if (list_empty(&cc->migratepages))
 		return;
 
-	list_for_each_entry(page, &cc->migratepages, lru)
-		count[!!page_is_file_cache(page)]++;
+	list_for_each_entry(page, &cc->migratepages, lru) {
+		if (PageLRU(page))
+			count[!!page_is_file_cache(page)]++;
+		else
+			count[2]++;
+	}
 
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
 }
 
 /* Similar to reclaim, but different enough that they don't share logic */
@@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
 			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
 	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
 			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
+	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
 	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
 			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
 
diff --git a/mm/migrate.c b/mm/migrate.c
index f7ee04a..cd5abb2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
 			continue;
 		}
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+		if (PageLRU(page))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
+		else
+			dec_node_page_state(page, NR_ISOLATED_NONLRU);
 		/*
 		 * We isolated non-lru movable page so here we can use
 		 * __PageMovable because LRU page's mapping cannot have
@@ -1121,8 +1124,11 @@ out:
 		 * restored.
 		 */
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+		if (PageLRU(page))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
+		else
+			dec_node_page_state(page, NR_ISOLATED_NONLRU);
 	}
 
 	/*
-- 
1.9.1

[PATCH] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[ming.ling]

Non-lru pages don't belong to any lru, so accounting them to
NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
It may misguide functions such as pgdat_reclaimable_pages and
too_many_isolated.

This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
And with non-lru pages in vmstat, it helps to optimize algorithm of
function too_many_isolated oneday.

Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
---
 include/linux/mmzone.h |  1 +
 mm/compaction.c        | 12 +++++++++---
 mm/migrate.c           | 14 ++++++++++----
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 7f2ae99..dc0adba 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -169,6 +169,7 @@ enum node_stat_item {
 	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
 	NR_DIRTIED,		/* page dirtyings since bootup */
 	NR_WRITTEN,		/* page writings since bootup */
+	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
 	NR_VM_NODE_STAT_ITEMS
 };
 
diff --git a/mm/compaction.c b/mm/compaction.c
index 9affb29..8da1dca 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
 static void acct_isolated(struct zone *zone, struct compact_control *cc)
 {
 	struct page *page;
-	unsigned int count[2] = { 0, };
+	unsigned int count[3] = { 0, };
 
 	if (list_empty(&cc->migratepages))
 		return;
 
-	list_for_each_entry(page, &cc->migratepages, lru)
-		count[!!page_is_file_cache(page)]++;
+	list_for_each_entry(page, &cc->migratepages, lru) {
+		if (PageLRU(page))
+			count[!!page_is_file_cache(page)]++;
+		else
+			count[2]++;
+	}
 
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
+	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
 }
 
 /* Similar to reclaim, but different enough that they don't share logic */
@@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
 			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
 	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
 			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
+	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
 	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
 			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
 
diff --git a/mm/migrate.c b/mm/migrate.c
index f7ee04a..cd5abb2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
 			continue;
 		}
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+		if (PageLRU(page))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
+		else
+			dec_node_page_state(page, NR_ISOLATED_NONLRU);
 		/*
 		 * We isolated non-lru movable page so here we can use
 		 * __PageMovable because LRU page's mapping cannot have
@@ -1121,8 +1124,11 @@ out:
 		 * restored.
 		 */
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+		if (PageLRU(page))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
+		else
+			dec_node_page_state(page, NR_ISOLATED_NONLRU);
 	}
 
 	/*
-- 
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: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Michal Hocko]

On Wed 28-09-16 17:31:03, ming.ling wrote:
> Non-lru pages don't belong to any lru, so accounting them to
> NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> It may misguide functions such as pgdat_reclaimable_pages and
> too_many_isolated.

OK, but it would be better to mention that this related to pfn based
migration (e.g. compaction). It is also highly appreciated to describe
the actual problem that you are seeing and tryin to fix. Is a reclaim
artificially throttled (hung) because of too many non LRU pages being
isolated? Is there a way to trigger that condition? So please tell us
more.
 
> This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> And with non-lru pages in vmstat, it helps to optimize algorithm of
> function too_many_isolated oneday.

I am not entirely sure a new vmstat counter is really needed but I have
to admit that the role of too_many_isolated in isolate_migratepages_block
is quite unclear to me. Besides that I believe the patch is not correct
because you are checking PageLRU after those pages have already been
isolated from the LRU. For example putback_movable_pages operates on
pages which are put back to the LRU. I haven't checked others but I
suspect they would be in a similar situation.

> Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> ---
>  include/linux/mmzone.h |  1 +
>  mm/compaction.c        | 12 +++++++++---
>  mm/migrate.c           | 14 ++++++++++----
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 7f2ae99..dc0adba 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -169,6 +169,7 @@ enum node_stat_item {
>  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
>  	NR_DIRTIED,		/* page dirtyings since bootup */
>  	NR_WRITTEN,		/* page writings since bootup */
> +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
>  	NR_VM_NODE_STAT_ITEMS
>  };
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9affb29..8da1dca 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
>  static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned int count[2] = { 0, };
> +	unsigned int count[3] = { 0, };
>  
>  	if (list_empty(&cc->migratepages))
>  		return;
>  
> -	list_for_each_entry(page, &cc->migratepages, lru)
> -		count[!!page_is_file_cache(page)]++;
> +	list_for_each_entry(page, &cc->migratepages, lru) {
> +		if (PageLRU(page))
> +			count[!!page_is_file_cache(page)]++;
> +		else
> +			count[2]++;
> +	}
>  
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
>  }
>  
>  /* Similar to reclaim, but different enough that they don't share logic */
> @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
>  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
>  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
>  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7ee04a..cd5abb2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
>  			continue;
>  		}
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -1121,8 +1124,11 @@ out:
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  	}
>  
>  	/*
> -- 
> 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>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Michal Hocko]

On Wed 28-09-16 17:31:03, ming.ling wrote:
> Non-lru pages don't belong to any lru, so accounting them to
> NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> It may misguide functions such as pgdat_reclaimable_pages and
> too_many_isolated.

OK, but it would be better to mention that this related to pfn based
migration (e.g. compaction). It is also highly appreciated to describe
the actual problem that you are seeing and tryin to fix. Is a reclaim
artificially throttled (hung) because of too many non LRU pages being
isolated? Is there a way to trigger that condition? So please tell us
more.
 
> This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> And with non-lru pages in vmstat, it helps to optimize algorithm of
> function too_many_isolated oneday.

I am not entirely sure a new vmstat counter is really needed but I have
to admit that the role of too_many_isolated in isolate_migratepages_block
is quite unclear to me. Besides that I believe the patch is not correct
because you are checking PageLRU after those pages have already been
isolated from the LRU. For example putback_movable_pages operates on
pages which are put back to the LRU. I haven't checked others but I
suspect they would be in a similar situation.

> Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> ---
>  include/linux/mmzone.h |  1 +
>  mm/compaction.c        | 12 +++++++++---
>  mm/migrate.c           | 14 ++++++++++----
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 7f2ae99..dc0adba 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -169,6 +169,7 @@ enum node_stat_item {
>  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
>  	NR_DIRTIED,		/* page dirtyings since bootup */
>  	NR_WRITTEN,		/* page writings since bootup */
> +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
>  	NR_VM_NODE_STAT_ITEMS
>  };
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9affb29..8da1dca 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
>  static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned int count[2] = { 0, };
> +	unsigned int count[3] = { 0, };
>  
>  	if (list_empty(&cc->migratepages))
>  		return;
>  
> -	list_for_each_entry(page, &cc->migratepages, lru)
> -		count[!!page_is_file_cache(page)]++;
> +	list_for_each_entry(page, &cc->migratepages, lru) {
> +		if (PageLRU(page))
> +			count[!!page_is_file_cache(page)]++;
> +		else
> +			count[2]++;
> +	}
>  
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
>  }
>  
>  /* Similar to reclaim, but different enough that they don't share logic */
> @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
>  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
>  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
>  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7ee04a..cd5abb2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
>  			continue;
>  		}
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -1121,8 +1124,11 @@ out:
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  	}
>  
>  	/*
> -- 
> 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>

-- 
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] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Minchan Kim]

Hello,

On Wed, Sep 28, 2016 at 05:31:03PM +0800, ming.ling wrote:
> Non-lru pages don't belong to any lru, so accounting them to
> NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> It may misguide functions such as pgdat_reclaimable_pages and
> too_many_isolated.

I agree this part. It would be happier if you give any story you suffered
from. Although you don't have, it's okay because you are correcting
clearly wrong part. Thanks. :)

> 
> This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> And with non-lru pages in vmstat, it helps to optimize algorithm of
> function too_many_isolated oneday.

Need more justfication to add new vmstat because once we add it, it's
really hard to change/remove it(i.e., maintainace trobule) so I want
to add it when it really would be helpful sometime, not now.

Could you resend the patch without part adding new vmstat?

Thanks.

> 
> Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> ---
>  include/linux/mmzone.h |  1 +
>  mm/compaction.c        | 12 +++++++++---
>  mm/migrate.c           | 14 ++++++++++----
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 7f2ae99..dc0adba 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -169,6 +169,7 @@ enum node_stat_item {
>  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
>  	NR_DIRTIED,		/* page dirtyings since bootup */
>  	NR_WRITTEN,		/* page writings since bootup */
> +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
>  	NR_VM_NODE_STAT_ITEMS
>  };
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9affb29..8da1dca 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
>  static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned int count[2] = { 0, };
> +	unsigned int count[3] = { 0, };
>  
>  	if (list_empty(&cc->migratepages))
>  		return;
>  
> -	list_for_each_entry(page, &cc->migratepages, lru)
> -		count[!!page_is_file_cache(page)]++;
> +	list_for_each_entry(page, &cc->migratepages, lru) {
> +		if (PageLRU(page))
> +			count[!!page_is_file_cache(page)]++;
> +		else
> +			count[2]++;
> +	}
>  
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
>  }
>  
>  /* Similar to reclaim, but different enough that they don't share logic */
> @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
>  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
>  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
>  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7ee04a..cd5abb2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
>  			continue;
>  		}
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -1121,8 +1124,11 @@ out:
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  	}
>  
>  	/*
> -- 
> 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: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Minchan Kim]

Hello,

On Wed, Sep 28, 2016 at 05:31:03PM +0800, ming.ling wrote:
> Non-lru pages don't belong to any lru, so accounting them to
> NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> It may misguide functions such as pgdat_reclaimable_pages and
> too_many_isolated.

I agree this part. It would be happier if you give any story you suffered
from. Although you don't have, it's okay because you are correcting
clearly wrong part. Thanks. :)

> 
> This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> And with non-lru pages in vmstat, it helps to optimize algorithm of
> function too_many_isolated oneday.

Need more justfication to add new vmstat because once we add it, it's
really hard to change/remove it(i.e., maintainace trobule) so I want
to add it when it really would be helpful sometime, not now.

Could you resend the patch without part adding new vmstat?

Thanks.

> 
> Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> ---
>  include/linux/mmzone.h |  1 +
>  mm/compaction.c        | 12 +++++++++---
>  mm/migrate.c           | 14 ++++++++++----
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 7f2ae99..dc0adba 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -169,6 +169,7 @@ enum node_stat_item {
>  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
>  	NR_DIRTIED,		/* page dirtyings since bootup */
>  	NR_WRITTEN,		/* page writings since bootup */
> +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
>  	NR_VM_NODE_STAT_ITEMS
>  };
>  
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 9affb29..8da1dca 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
>  static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  {
>  	struct page *page;
> -	unsigned int count[2] = { 0, };
> +	unsigned int count[3] = { 0, };
>  
>  	if (list_empty(&cc->migratepages))
>  		return;
>  
> -	list_for_each_entry(page, &cc->migratepages, lru)
> -		count[!!page_is_file_cache(page)]++;
> +	list_for_each_entry(page, &cc->migratepages, lru) {
> +		if (PageLRU(page))
> +			count[!!page_is_file_cache(page)]++;
> +		else
> +			count[2]++;
> +	}
>  
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
>  }
>  
>  /* Similar to reclaim, but different enough that they don't share logic */
> @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
>  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
>  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
>  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
>  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f7ee04a..cd5abb2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
>  			continue;
>  		}
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -1121,8 +1124,11 @@ out:
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (PageLRU(page))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +		else
> +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
>  	}
>  
>  	/*
> -- 
> 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>

--
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: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Ming Ling]

On 四,  9月 29, 2016 at 10:01:43上午 +0200, Michal Hocko wrote:
> On Wed 28-09-16 17:31:03, ming.ling wrote:
> > Non-lru pages don't belong to any lru, so accounting them to
> > NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> > It may misguide functions such as pgdat_reclaimable_pages and
> > too_many_isolated.
> 
> OK, but it would be better to mention that this related to pfn based
> migration (e.g. compaction). It is also highly appreciated to describe
> the actual problem that you are seeing and tryin to fix. Is a reclaim
> artificially throttled (hung) because of too many non LRU pages being
> isolated? Is there a way to trigger that condition? So please tell us
> more.
>
Yes, you are right, it only related to pfn based migration such as
compaction and alloc_contig_range. On mobile devices such as 512M ram
android Phone, it may use a big zram swap. In some case zram(zsmalloc) 
use too many non-lru pages,such as:
	MemTotal: 468148 kB
	Normal free:5620kB
	Free swap:4736kB
	Total swap:409596kB
	ZRAM: 164616kB(zsmalloc non-lru pages)
	active_anon:60700kB
	inactive_anon:60744kB
	active_file:34420kB
	inactive_file:37532kB
More non-lru pages which used by zram for swap, it influences
pgdat_reclaimable_pages and too_many_isolated more. But it is very hard
to make a special case making reclaim artificially throttled (hung) 
because of too many non LRU pages being isolated. Larger swap disk,
higher swapness, more backup applications, lower lowmemory killer minfree
water level, more high order pageblock allocation may trigger that condition.  
> > This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> > pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> > And with non-lru pages in vmstat, it helps to optimize algorithm of
> > function too_many_isolated oneday.
> 
> I am not entirely sure a new vmstat counter is really needed but I have
> to admit that the role of too_many_isolated in isolate_migratepages_block
> is quite unclear to me. Besides that I believe the patch is not correct
> because you are checking PageLRU after those pages have already been
> isolated from the LRU. For example putback_movable_pages operates on
> pages which are put back to the LRU. I haven't checked others but I
> suspect they would be in a similar situation.
> 
Yes, i had make a mistake. After those pages have already been isolated from
the LRU, it has to check __PageMovable instead of PageLRU such as function
putback_movable_pages, unmap_and_move and __unmap_and_move.
I will correct it in next patch,. Thank you very much.
> > Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> > ---
> >  include/linux/mmzone.h |  1 +
> >  mm/compaction.c        | 12 +++++++++---
> >  mm/migrate.c           | 14 ++++++++++----
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 7f2ae99..dc0adba 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -169,6 +169,7 @@ enum node_stat_item {
> >  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
> >  	NR_DIRTIED,		/* page dirtyings since bootup */
> >  	NR_WRITTEN,		/* page writings since bootup */
> > +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
> >  	NR_VM_NODE_STAT_ITEMS
> >  };
> >  
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 9affb29..8da1dca 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
> >  static void acct_isolated(struct zone *zone, struct compact_control *cc)
> >  {
> >  	struct page *page;
> > -	unsigned int count[2] = { 0, };
> > +	unsigned int count[3] = { 0, };
> >  
> >  	if (list_empty(&cc->migratepages))
> >  		return;
> >  
> > -	list_for_each_entry(page, &cc->migratepages, lru)
> > -		count[!!page_is_file_cache(page)]++;
> > +	list_for_each_entry(page, &cc->migratepages, lru) {
> > +		if (PageLRU(page))
> > +			count[!!page_is_file_cache(page)]++;
> > +		else
> > +			count[2]++;
> > +	}
> >  
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> > +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
> >  }
> >  
> >  /* Similar to reclaim, but different enough that they don't share logic */
> > @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
> >  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
> >  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> > +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
> >  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
> >  
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7ee04a..cd5abb2 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
> >  			continue;
> >  		}
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  		/*
> >  		 * We isolated non-lru movable page so here we can use
> >  		 * __PageMovable because LRU page's mapping cannot have
> > @@ -1121,8 +1124,11 @@ out:
> >  		 * restored.
> >  		 */
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  	}
> >  
> >  	/*
> > -- 
> > 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>
> 
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Ming Ling]

On a??,  9ae?? 29, 2016 at 10:01:43a,?a?? +0200, Michal Hocko wrote:
> On Wed 28-09-16 17:31:03, ming.ling wrote:
> > Non-lru pages don't belong to any lru, so accounting them to
> > NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> > It may misguide functions such as pgdat_reclaimable_pages and
> > too_many_isolated.
> 
> OK, but it would be better to mention that this related to pfn based
> migration (e.g. compaction). It is also highly appreciated to describe
> the actual problem that you are seeing and tryin to fix. Is a reclaim
> artificially throttled (hung) because of too many non LRU pages being
> isolated? Is there a way to trigger that condition? So please tell us
> more.
>
Yes, you are right, it only related to pfn based migration such as
compaction and alloc_contig_range. On mobile devices such as 512M ram
android Phone, it may use a big zram swap. In some case zram(zsmalloc) 
use too many non-lru pages,such as:
	MemTotal: 468148 kB
	Normal free:5620kB
	Free swap:4736kB
	Total swap:409596kB
	ZRAM: 164616kB(zsmalloc non-lru pages)
	active_anon:60700kB
	inactive_anon:60744kB
	active_file:34420kB
	inactive_file:37532kB
More non-lru pages which used by zram for swap, it influences
pgdat_reclaimable_pages and too_many_isolated more. But it is very hard
to make a special case making reclaim artificially throttled (hung) 
because of too many non LRU pages being isolated. Larger swap disk,
higher swapness, more backup applications, lower lowmemory killer minfree
water level, more high order pageblock allocation may trigger that condition.  
> > This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> > pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> > And with non-lru pages in vmstat, it helps to optimize algorithm of
> > function too_many_isolated oneday.
> 
> I am not entirely sure a new vmstat counter is really needed but I have
> to admit that the role of too_many_isolated in isolate_migratepages_block
> is quite unclear to me. Besides that I believe the patch is not correct
> because you are checking PageLRU after those pages have already been
> isolated from the LRU. For example putback_movable_pages operates on
> pages which are put back to the LRU. I haven't checked others but I
> suspect they would be in a similar situation.
> 
Yes, i had make a mistake. After those pages have already been isolated from
the LRU, it has to check __PageMovable instead of PageLRU such as function
putback_movable_pages, unmap_and_move and __unmap_and_move.
I will correct it in next patch,. Thank you very much.
> > Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> > ---
> >  include/linux/mmzone.h |  1 +
> >  mm/compaction.c        | 12 +++++++++---
> >  mm/migrate.c           | 14 ++++++++++----
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 7f2ae99..dc0adba 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -169,6 +169,7 @@ enum node_stat_item {
> >  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
> >  	NR_DIRTIED,		/* page dirtyings since bootup */
> >  	NR_WRITTEN,		/* page writings since bootup */
> > +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
> >  	NR_VM_NODE_STAT_ITEMS
> >  };
> >  
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 9affb29..8da1dca 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
> >  static void acct_isolated(struct zone *zone, struct compact_control *cc)
> >  {
> >  	struct page *page;
> > -	unsigned int count[2] = { 0, };
> > +	unsigned int count[3] = { 0, };
> >  
> >  	if (list_empty(&cc->migratepages))
> >  		return;
> >  
> > -	list_for_each_entry(page, &cc->migratepages, lru)
> > -		count[!!page_is_file_cache(page)]++;
> > +	list_for_each_entry(page, &cc->migratepages, lru) {
> > +		if (PageLRU(page))
> > +			count[!!page_is_file_cache(page)]++;
> > +		else
> > +			count[2]++;
> > +	}
> >  
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> > +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
> >  }
> >  
> >  /* Similar to reclaim, but different enough that they don't share logic */
> > @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
> >  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
> >  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> > +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
> >  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
> >  
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7ee04a..cd5abb2 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
> >  			continue;
> >  		}
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  		/*
> >  		 * We isolated non-lru movable page so here we can use
> >  		 * __PageMovable because LRU page's mapping cannot have
> > @@ -1121,8 +1124,11 @@ out:
> >  		 * restored.
> >  		 */
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  	}
> >  
> >  	/*
> > -- 
> > 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>
> 
> -- 
> 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] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Ming Ling]

Hello,

On 五,  9月 30, 2016 at 11:37:37上午 +0900, Minchan Kim wrote:
> Hello,
> 
> On Wed, Sep 28, 2016 at 05:31:03PM +0800, ming.ling wrote:
> > Non-lru pages don't belong to any lru, so accounting them to
> > NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> > It may misguide functions such as pgdat_reclaimable_pages and
> > too_many_isolated.
> 
> I agree this part. It would be happier if you give any story you suffered
> from. Although you don't have, it's okay because you are correcting
> clearly wrong part. Thanks. :)
> 
I haven't suffered from any actual problem yet. It is very hard to make a
special case making reclaim artificially throttled (hung) because of too
many non LRU pages being isolated. But since i do some develpments on low ram
Android devices（512M...), i am very sensitive on memory footprint analysis.
Incorrect counting of non-lru pages makes me confused on it.
And patch "mm, vmscan: consider isolated pages in zone_reclaimable_pages
(9f6c399d)" which had been committed by Michal Hocko adds isolated pages in
zone_reclaimable_pages. So i think it is worth to ensure their counts are right.
> > 
> > This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> > pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> > And with non-lru pages in vmstat, it helps to optimize algorithm of
> > function too_many_isolated oneday.
> 
> Need more justfication to add new vmstat because once we add it, it's
> really hard to change/remove it(i.e., maintainace trobule) so I want
> to add it when it really would be helpful sometime, not now.
> 
> Could you resend the patch without part adding new vmstat?
> 
> Thanks.
>
No problem, let's add it when it really would be helpful sometime.
I will resend the patch without part adding new vmstat later.

Thank you very much. 
> > 
> > Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> > ---
> >  include/linux/mmzone.h |  1 +
> >  mm/compaction.c        | 12 +++++++++---
> >  mm/migrate.c           | 14 ++++++++++----
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 7f2ae99..dc0adba 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -169,6 +169,7 @@ enum node_stat_item {
> >  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
> >  	NR_DIRTIED,		/* page dirtyings since bootup */
> >  	NR_WRITTEN,		/* page writings since bootup */
> > +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
> >  	NR_VM_NODE_STAT_ITEMS
> >  };
> >  
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 9affb29..8da1dca 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
> >  static void acct_isolated(struct zone *zone, struct compact_control *cc)
> >  {
> >  	struct page *page;
> > -	unsigned int count[2] = { 0, };
> > +	unsigned int count[3] = { 0, };
> >  
> >  	if (list_empty(&cc->migratepages))
> >  		return;
> >  
> > -	list_for_each_entry(page, &cc->migratepages, lru)
> > -		count[!!page_is_file_cache(page)]++;
> > +	list_for_each_entry(page, &cc->migratepages, lru) {
> > +		if (PageLRU(page))
> > +			count[!!page_is_file_cache(page)]++;
> > +		else
> > +			count[2]++;
> > +	}
> >  
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> > +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
> >  }
> >  
> >  /* Similar to reclaim, but different enough that they don't share logic */
> > @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
> >  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
> >  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> > +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
> >  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
> >  
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7ee04a..cd5abb2 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
> >  			continue;
> >  		}
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  		/*
> >  		 * We isolated non-lru movable page so here we can use
> >  		 * __PageMovable because LRU page's mapping cannot have
> > @@ -1121,8 +1124,11 @@ out:
> >  		 * restored.
> >  		 */
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  	}
> >  
> >  	/*
> > -- 
> > 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: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.

[Ming Ling]

Hello,

On ao?,  9ae?? 30, 2016 at 11:37:37a,?a?? +0900, Minchan Kim wrote:
> Hello,
> 
> On Wed, Sep 28, 2016 at 05:31:03PM +0800, ming.ling wrote:
> > Non-lru pages don't belong to any lru, so accounting them to
> > NR_ISOLATED_ANON or NR_ISOLATED_FILE doesn't make any sense.
> > It may misguide functions such as pgdat_reclaimable_pages and
> > too_many_isolated.
> 
> I agree this part. It would be happier if you give any story you suffered
> from. Although you don't have, it's okay because you are correcting
> clearly wrong part. Thanks. :)
> 
I haven't suffered from any actual problem yet. It is very hard to make a
special case making reclaim artificially throttled (hung) because of too
many non LRU pages being isolated. But since i do some develpments on low ram
Android devicesi 1/4 ?512M...), i am very sensitive on memory footprint analysis.
Incorrect counting of non-lru pages makes me confused on it.
And patch "mm, vmscan: consider isolated pages in zone_reclaimable_pages
(9f6c399d)" which had been committed by Michal Hocko adds isolated pages in
zone_reclaimable_pages. So i think it is worth to ensure their counts are right.
> > 
> > This patch adds NR_ISOLATED_NONLRU to vmstat and moves isolated non-lru
> > pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE to NR_ISOLATED_NONLRU.
> > And with non-lru pages in vmstat, it helps to optimize algorithm of
> > function too_many_isolated oneday.
> 
> Need more justfication to add new vmstat because once we add it, it's
> really hard to change/remove it(i.e., maintainace trobule) so I want
> to add it when it really would be helpful sometime, not now.
> 
> Could you resend the patch without part adding new vmstat?
> 
> Thanks.
>
No problem, let's add it when it really would be helpful sometime.
I will resend the patch without part adding new vmstat later.

Thank you very much. 
> > 
> > Signed-off-by: ming.ling <ming.ling@spreadtrum.com>
> > ---
> >  include/linux/mmzone.h |  1 +
> >  mm/compaction.c        | 12 +++++++++---
> >  mm/migrate.c           | 14 ++++++++++----
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 7f2ae99..dc0adba 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -169,6 +169,7 @@ enum node_stat_item {
> >  	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
> >  	NR_DIRTIED,		/* page dirtyings since bootup */
> >  	NR_WRITTEN,		/* page writings since bootup */
> > +	NR_ISOLATED_NONLRU,	/* Temporary isolated pages from non-lru */
> >  	NR_VM_NODE_STAT_ITEMS
> >  };
> >  
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 9affb29..8da1dca 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -638,16 +638,21 @@ isolate_freepages_range(struct compact_control *cc,
> >  static void acct_isolated(struct zone *zone, struct compact_control *cc)
> >  {
> >  	struct page *page;
> > -	unsigned int count[2] = { 0, };
> > +	unsigned int count[3] = { 0, };
> >  
> >  	if (list_empty(&cc->migratepages))
> >  		return;
> >  
> > -	list_for_each_entry(page, &cc->migratepages, lru)
> > -		count[!!page_is_file_cache(page)]++;
> > +	list_for_each_entry(page, &cc->migratepages, lru) {
> > +		if (PageLRU(page))
> > +			count[!!page_is_file_cache(page)]++;
> > +		else
> > +			count[2]++;
> > +	}
> >  
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> > +	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_NONLRU, count[2]);
> >  }
> >  
> >  /* Similar to reclaim, but different enough that they don't share logic */
> > @@ -659,6 +664,7 @@ static bool too_many_isolated(struct zone *zone)
> >  			node_page_state(zone->zone_pgdat, NR_INACTIVE_ANON);
> >  	active = node_page_state(zone->zone_pgdat, NR_ACTIVE_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ACTIVE_ANON);
> > +	/* Is it necessary to add NR_ISOLATED_NONLRU?? */
> >  	isolated = node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE) +
> >  			node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON);
> >  
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f7ee04a..cd5abb2 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -168,8 +168,11 @@ void putback_movable_pages(struct list_head *l)
> >  			continue;
> >  		}
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  		/*
> >  		 * We isolated non-lru movable page so here we can use
> >  		 * __PageMovable because LRU page's mapping cannot have
> > @@ -1121,8 +1124,11 @@ out:
> >  		 * restored.
> >  		 */
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (PageLRU(page))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> > +		else
> > +			dec_node_page_state(page, NR_ISOLATED_NONLRU);
> >  	}
> >  
> >  	/*
> > -- 
> > 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>

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