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

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

[ming.ling]

From: Ming Ling <ming.ling@spreadtrum.com>

Non-lru pages don't belong to any lru, so counting 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.
On mobile devices such as 512M ram android Phone, it may use
a big zram swap. In some cases zram(zsmalloc) uses 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.
This patch excludes isolated non-lru pages from NR_ISOLATED_ANON
or NR_ISOLATED_FILE to ensure their counts are right.

Signed-off-by: Ming ling <ming.ling@spreadtrum.com>
---
 mm/compaction.c | 6 ++++--
 mm/migrate.c    | 9 +++++----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 0409a4a..ed4c553 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -643,8 +643,10 @@ static void acct_isolated(struct zone *zone, struct compact_control *cc)
 	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 (likely(!__PageMovable(page)))
+			count[!!page_is_file_cache(page)]++;
+	}
 
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
 	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
diff --git a/mm/migrate.c b/mm/migrate.c
index 99250ae..abe48cc 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -168,8 +168,6 @@ 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));
 		/*
 		 * We isolated non-lru movable page so here we can use
 		 * __PageMovable because LRU page's mapping cannot have
@@ -185,6 +183,8 @@ void putback_movable_pages(struct list_head *l)
 			unlock_page(page);
 			put_page(page);
 		} else {
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 			putback_lru_page(page);
 		}
 	}
@@ -1121,8 +1121,9 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 		 * restored.
 		 */
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+		if (likely(!__PageMovable(page)))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 	}
 
 	/*
-- 
1.9.1

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

[Michal Hocko]

On Thu 13-10-16 14:39:09, ming.ling wrote:
> From: Ming Ling <ming.ling@spreadtrum.com>
> 
> Non-lru pages don't belong to any lru, so counting 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.

That doesn't make much sense to me. I guess you wanted to say something
like
"
Accounting non-lru pages isolated for migration during pfn walk to
NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
heuristics based on those counters such as pgdat_reclaimable_pages resp.
too_many_isolated. Note that __alloc_contig_migrate_range can isolate
a lot of pages at once.
"
> On mobile devices such as 512M ram android Phone, it may use
> a big zram swap. In some cases zram(zsmalloc) uses 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

I assume those zsmalloc pages are migrateable and that is the problem?
Please state that explicitly so that even people not familiar with
zsmalloc understand the motivation.

> More non-lru pages which used by zram for swap, it influences
> pgdat_reclaimable_pages and too_many_isolated more.

It would be good to mention what would be a visible effect of this.
"If the NR_ISOLATED_* is too large then the direct reclaim might get
throttled prematurely inducing longer allocation latencies without any
strong reason."

> This patch excludes isolated non-lru pages from NR_ISOLATED_ANON
> or NR_ISOLATED_FILE to ensure their counts are right.

But this patch doesn't do that. It just relies on __PageMovable. It is
true that all LRU pages should be movable (well except for
NR_UNEVICTABLE in certain configurations) but is it true that all
movable pages are on the LRU list?

Why don't you simply mimic what shrink_inactive_list does? Aka count the
number of isolated pages and then account them when appropriate?
 
> Signed-off-by: Ming ling <ming.ling@spreadtrum.com>
> ---
>  mm/compaction.c | 6 ++++--
>  mm/migrate.c    | 9 +++++----
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 0409a4a..ed4c553 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -643,8 +643,10 @@ static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  	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 (likely(!__PageMovable(page)))
> +			count[!!page_is_file_cache(page)]++;
> +	}
>  
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
>  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 99250ae..abe48cc 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,6 @@ 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));
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -185,6 +183,8 @@ void putback_movable_pages(struct list_head *l)
>  			unlock_page(page);
>  			put_page(page);
>  		} else {
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
>  			putback_lru_page(page);
>  		}
>  	}
> @@ -1121,8 +1121,9 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +		if (likely(!__PageMovable(page)))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
>  	}
>  
>  	/*
> -- 
> 1.9.1

-- 
Michal Hocko
SUSE Labs

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

[Ming Ling]

On 四, 10月 13, 2016 at 10:09:37上午 +0200, Michal Hocko wrote:
Hello,
> On Thu 13-10-16 14:39:09, ming.ling wrote:
> > From: Ming Ling <ming.ling@spreadtrum.com>
> > 
> > Non-lru pages don't belong to any lru, so counting 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.
> 
> That doesn't make much sense to me. I guess you wanted to say something
> like
> "
> Accounting non-lru pages isolated for migration during pfn walk to
> NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
> heuristics based on those counters such as pgdat_reclaimable_pages resp.
> too_many_isolated. Note that __alloc_contig_migrate_range can isolate
> a lot of pages at once.
> "
Yes，your understanding is right, and your description is clearer than
mine. Do your mind if i borrow it as a comment of this patch in next
version?
> > On mobile devices such as 512M ram android Phone, it may use
> > a big zram swap. In some cases zram(zsmalloc) uses 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
> 
> I assume those zsmalloc pages are migrateable and that is the problem?
> Please state that explicitly so that even people not familiar with
> zsmalloc understand the motivation.

Yes, since Minchan Kim had committed ‘mm: migrate: support non-lru
movable page migration’, those zsmalloc pages are migrateable now.
And i will state that explicitly in next version.
> 
> > More non-lru pages which used by zram for swap, it influences
> > pgdat_reclaimable_pages and too_many_isolated more.
> 
> It would be good to mention what would be a visible effect of this.
> "If the NR_ISOLATED_* is too large then the direct reclaim might get
> throttled prematurely inducing longer allocation latencies without any
> strong reason."
> 
I will detail the effect of counting so many non-lru pages into
NR_ISOLATED_{ANON,FILE} such as:
'In function shrink_inactive_list, if there are too many isolated
pages,it will wait for a moment. So If we miscounting large number
non-lru pages into NR_ISOLATED_{ANON,FILE}, direct reclaim might
getthrottled prematurely inducing longer allocation latencies
without any strong reason. Actually there is no need to take non-lru
pages into account in shrink_inactive_list which just deals with
lru pages.

In function pgdat_reclaimable_pages, you had considered isolated
pages in zone_reclaimable_pages. So miscounting non-lru pages into
NR_ISOLATED_{ANON,FILE} also larger zone_reclaimable_pages and will
lead to a more optimistic zone_reclaimable judgement.
'
> > This patch excludes isolated non-lru pages from NR_ISOLATED_ANON
> > or NR_ISOLATED_FILE to ensure their counts are right.
> 
> But this patch doesn't do that. It just relies on __PageMovable. It is
> true that all LRU pages should be movable (well except for
> NR_UNEVICTABLE in certain configurations) but is it true that all
> movable pages are on the LRU list?
>
I don't think so. In commit bda807d4 'mm: migrate: support non-lru
movable page migration', Minchan Kim point out :
'For testing of non-lru movable page, VM supports __PageMovable function.
However, it doesn't guarantee to identify non-lru movable page because
page->mapping field is unified with other variables in struct page.  As
well, if driver releases the page after isolation by VM, page->mapping
doesn't have stable value although it has PAGE_MAPPING_MOVABLE (Look at
__ClearPageMovable).  But __PageMovable is cheap to catch whether page
is LRU or non-lru movable once the page has been isolated.  Because LRU
pages never can have PAGE_MAPPING_MOVABLE in page->mapping.  It is also
good for just peeking to test non-lru movable pages before more
expensive checking with lock_page in pfn scanning to select victim.'.

And he uses __PageMovable to judge whether a isolated page is a lru page
such as:
void putback_movable_pages(struct list_head *l)
{
	......
	/*
	 * We isolated non-lru movable page so here we can use
 	 * __PageMovable because LRU page's mapping cannot have
	 * PAGE_MAPPING_MOVABLE.
	 */
	if (unlikely(__PageMovable(page))) {
		VM_BUG_ON_PAGE(!PageIsolated(page), page);
		lock_page(page);
		if (PageMovable(page))
			putback_movable_page(page);
		else
			__ClearPageIsolated(page);
		unlock_page(page);
		put_page(page);
	} else {
		putback_lru_page(page);
	}
}
 
> Why don't you simply mimic what shrink_inactive_list does? Aka count the
> number of isolated pages and then account them when appropriate?
>
I think i am correcting clearly wrong part. So, there is no need to
describe it too detailed. It's a misunderstanding, and i will add
more comments as you suggest.

I am looking forward to more suggestions from you.
Thank you very much. 
> > Signed-off-by: Ming ling <ming.ling@spreadtrum.com>
> > ---
> >  mm/compaction.c | 6 ++++--
> >  mm/migrate.c    | 9 +++++----
> >  2 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 0409a4a..ed4c553 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -643,8 +643,10 @@ static void acct_isolated(struct zone *zone, struct compact_control *cc)
> >  	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 (likely(!__PageMovable(page)))
> > +			count[!!page_is_file_cache(page)]++;
> > +	}
> >  
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> >  	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 99250ae..abe48cc 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -168,8 +168,6 @@ 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));
> >  		/*
> >  		 * We isolated non-lru movable page so here we can use
> >  		 * __PageMovable because LRU page's mapping cannot have
> > @@ -185,6 +183,8 @@ void putback_movable_pages(struct list_head *l)
> >  			unlock_page(page);
> >  			put_page(page);
> >  		} else {
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> >  			putback_lru_page(page);
> >  		}
> >  	}
> > @@ -1121,8 +1121,9 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
> >  		 * restored.
> >  		 */
> >  		list_del(&page->lru);
> > -		dec_node_page_state(page, NR_ISOLATED_ANON +
> > -				page_is_file_cache(page));
> > +		if (likely(!__PageMovable(page)))
> > +			dec_node_page_state(page, NR_ISOLATED_ANON +
> > +					page_is_file_cache(page));
> >  	}
> >  
> >  	/*
> > -- 
> > 1.9.1
> 
> -- 
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Fri 14-10-16 16:32:19, Ming Ling wrote:
> On 四, 10月 13, 2016 at 10:09:37上午 +0200, Michal Hocko wrote:
> Hello,
> > On Thu 13-10-16 14:39:09, ming.ling wrote:
> > > From: Ming Ling <ming.ling@spreadtrum.com>
> > > 
> > > Non-lru pages don't belong to any lru, so counting 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.
> > 
> > That doesn't make much sense to me. I guess you wanted to say something
> > like
> > "
> > Accounting non-lru pages isolated for migration during pfn walk to
> > NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
> > heuristics based on those counters such as pgdat_reclaimable_pages resp.
> > too_many_isolated. Note that __alloc_contig_migrate_range can isolate
> > a lot of pages at once.
> > "
> Yes，your understanding is right, and your description is clearer than
> mine. Do your mind if i borrow it as a comment of this patch in next
> version?

sure, go ahead

> > > On mobile devices such as 512M ram android Phone, it may use
> > > a big zram swap. In some cases zram(zsmalloc) uses 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
> > 
> > I assume those zsmalloc pages are migrateable and that is the problem?
> > Please state that explicitly so that even people not familiar with
> > zsmalloc understand the motivation.
> 
> Yes, since Minchan Kim had committed ‘mm: migrate: support non-lru
> movable page migration’, those zsmalloc pages are migrateable now.
> And i will state that explicitly in next version.

OK

> > > More non-lru pages which used by zram for swap, it influences
> > > pgdat_reclaimable_pages and too_many_isolated more.
> > 
> > It would be good to mention what would be a visible effect of this.
> > "If the NR_ISOLATED_* is too large then the direct reclaim might get
> > throttled prematurely inducing longer allocation latencies without any
> > strong reason."
> > 
> I will detail the effect of counting so many non-lru pages into
> NR_ISOLATED_{ANON,FILE} such as:
>
> 'In function shrink_inactive_list, if there are too many isolated
> pages,it will wait for a moment. So If we miscounting large number
> non-lru pages into NR_ISOLATED_{ANON,FILE}, direct reclaim might
> getthrottled prematurely inducing longer allocation latencies
> without any strong reason. Actually there is no need to take non-lru
> pages into account in shrink_inactive_list which just deals with
> lru pages.

Note that this is true also for the direct compaction.

> In function pgdat_reclaimable_pages, you had considered isolated
> pages in zone_reclaimable_pages. So miscounting non-lru pages into
> NR_ISOLATED_{ANON,FILE} also larger zone_reclaimable_pages and will
> lead to a more optimistic zone_reclaimable judgement.

Which shouldn't be such a big deal.

> '
> > > This patch excludes isolated non-lru pages from NR_ISOLATED_ANON
> > > or NR_ISOLATED_FILE to ensure their counts are right.
> > 
> > But this patch doesn't do that. It just relies on __PageMovable. It is
> > true that all LRU pages should be movable (well except for
> > NR_UNEVICTABLE in certain configurations) but is it true that all
> > movable pages are on the LRU list?
> >
>
> I don't think so. In commit bda807d4 'mm: migrate: support non-lru
> movable page migration', Minchan Kim point out :
> 'For testing of non-lru movable page, VM supports __PageMovable function.
> However, it doesn't guarantee to identify non-lru movable page because
> page->mapping field is unified with other variables in struct page.  As
> well, if driver releases the page after isolation by VM, page->mapping
> doesn't have stable value although it has PAGE_MAPPING_MOVABLE (Look at
> __ClearPageMovable).  But __PageMovable is cheap to catch whether page
> is LRU or non-lru movable once the page has been isolated.  Because LRU
> pages never can have PAGE_MAPPING_MOVABLE in page->mapping.  It is also
> good for just peeking to test non-lru movable pages before more
> expensive checking with lock_page in pfn scanning to select victim.'.
> 
> And he uses __PageMovable to judge whether a isolated page is a lru page
> such as:
> void putback_movable_pages(struct list_head *l)
> {
> 	......
> 	/*
> 	 * We isolated non-lru movable page so here we can use
>  	 * __PageMovable because LRU page's mapping cannot have
> 	 * PAGE_MAPPING_MOVABLE.
> 	 */
> 	if (unlikely(__PageMovable(page))) {
> 		VM_BUG_ON_PAGE(!PageIsolated(page), page);
> 		lock_page(page);
> 		if (PageMovable(page))
> 			putback_movable_page(page);
> 		else
> 			__ClearPageIsolated(page);
> 		unlock_page(page);
> 		put_page(page);
> 	} else {
> 		putback_lru_page(page);
> 	}
> }

I am not familiar with this code enough to comment but to me it all
sounds quite subtle.

> > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > number of isolated pages and then account them when appropriate?
> >
> I think i am correcting clearly wrong part. So, there is no need to
> describe it too detailed. It's a misunderstanding, and i will add
> more comments as you suggest.

OK, so could you explain why you prefer to relyon __PageMovable rather
than do a trivial counting during the isolation?
-- 
Michal Hocko
SUSE Labs

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

[Minchan Kim]

Hi, Michal,

On Fri, Oct 14, 2016 at 01:30:44PM +0200, Michal Hocko wrote:

< snip>

> > void putback_movable_pages(struct list_head *l)
> > {
> > 	......
> > 	/*
> > 	 * We isolated non-lru movable page so here we can use
> >  	 * __PageMovable because LRU page's mapping cannot have
> > 	 * PAGE_MAPPING_MOVABLE.
> > 	 */
> > 	if (unlikely(__PageMovable(page))) {
> > 		VM_BUG_ON_PAGE(!PageIsolated(page), page);
> > 		lock_page(page);
> > 		if (PageMovable(page))
> > 			putback_movable_page(page);
> > 		else
> > 			__ClearPageIsolated(page);
> > 		unlock_page(page);
> > 		put_page(page);
> > 	} else {
> > 		putback_lru_page(page);
> > 	}
> > }
> 
> I am not familiar with this code enough to comment but to me it all
> sounds quite subtle.

It was due to lacking of page flags on 32bit machine, sadly.
Better idea is always welcome.

> 
> > > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > > number of isolated pages and then account them when appropriate?
> > >
> > I think i am correcting clearly wrong part. So, there is no need to
> > describe it too detailed. It's a misunderstanding, and i will add
> > more comments as you suggest.
> 
> OK, so could you explain why you prefer to relyon __PageMovable rather
> than do a trivial counting during the isolation?

I don't get it. Could you elaborate it a bit more?

> -- 
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Fri 14-10-16 22:46:04, Minchan Kim wrote:
[...]
> > > > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > > > number of isolated pages and then account them when appropriate?
> > > >
> > > I think i am correcting clearly wrong part. So, there is no need to
> > > describe it too detailed. It's a misunderstanding, and i will add
> > > more comments as you suggest.
> > 
> > OK, so could you explain why you prefer to relyon __PageMovable rather
> > than do a trivial counting during the isolation?
> 
> I don't get it. Could you elaborate it a bit more?

It is really simple. You can count the number of file and anonymous
pages while they are isolated and then account them to NR_ISOLATED_*
later. Basically the same thing we do during the reclaim. We absolutely
do not have to rely on __PageMovable and make this code more complex
than necessary.
-- 
Michal Hocko
SUSE Labs

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

[Minchan Kim]

On Fri, Oct 14, 2016 at 03:53:34PM +0200, Michal Hocko wrote:
> On Fri 14-10-16 22:46:04, Minchan Kim wrote:
> [...]
> > > > > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > > > > number of isolated pages and then account them when appropriate?
> > > > >
> > > > I think i am correcting clearly wrong part. So, there is no need to
> > > > describe it too detailed. It's a misunderstanding, and i will add
> > > > more comments as you suggest.
> > > 
> > > OK, so could you explain why you prefer to relyon __PageMovable rather
> > > than do a trivial counting during the isolation?
> > 
> > I don't get it. Could you elaborate it a bit more?
> 
> It is really simple. You can count the number of file and anonymous
> pages while they are isolated and then account them to NR_ISOLATED_*
> later. Basically the same thing we do during the reclaim. We absolutely
> do not have to rely on __PageMovable and make this code more complex
> than necessary.

I don't understand your point.
isolate_migratepages_block can isolate any movable pages, for instance,
anon, file and non-lru and they are isolated into cc->migratepges.
Then, acct_isolated accounts them to NR_ISOLATED_*.
Isn't it same with the one you suggested?
The problem is we should identify which pages is non-lru movable first.
If it's not non-lru, it means the page is either anon or file so we
can account them. 
That's exactly waht Ming Ling did.

Sorry if I didn't get your point. Maybe, it would be better to give
pseudo code out of your mind for better understanding rather than
several ping-ping with vague words.

Thanks.

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

[Michal Hocko]

On Fri 14-10-16 23:44:48, Minchan Kim wrote:
> On Fri, Oct 14, 2016 at 03:53:34PM +0200, Michal Hocko wrote:
> > On Fri 14-10-16 22:46:04, Minchan Kim wrote:
> > [...]
> > > > > > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > > > > > number of isolated pages and then account them when appropriate?
> > > > > >
> > > > > I think i am correcting clearly wrong part. So, there is no need to
> > > > > describe it too detailed. It's a misunderstanding, and i will add
> > > > > more comments as you suggest.
> > > > 
> > > > OK, so could you explain why you prefer to relyon __PageMovable rather
> > > > than do a trivial counting during the isolation?
> > > 
> > > I don't get it. Could you elaborate it a bit more?
> > 
> > It is really simple. You can count the number of file and anonymous
> > pages while they are isolated and then account them to NR_ISOLATED_*
> > later. Basically the same thing we do during the reclaim. We absolutely
> > do not have to rely on __PageMovable and make this code more complex
> > than necessary.
> 
> I don't understand your point.
> isolate_migratepages_block can isolate any movable pages, for instance,
> anon, file and non-lru and they are isolated into cc->migratepges.
> Then, acct_isolated accounts them to NR_ISOLATED_*.
> Isn't it same with the one you suggested?
> The problem is we should identify which pages is non-lru movable first.
> If it's not non-lru, it means the page is either anon or file so we
> can account them. 
> That's exactly waht Ming Ling did.
> 
> Sorry if I didn't get your point. Maybe, it would be better to give
> pseudo code out of your mind for better understanding rather than
> several ping-ping with vague words.

diff --git a/mm/compaction.c b/mm/compaction.c
index 0409a4ad6ea1..6584705a46f6 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
  */
 static unsigned long
 isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
-			unsigned long end_pfn, isolate_mode_t isolate_mode)
+			unsigned long end_pfn, isolate_mode_t isolate_mode,
+			unsigned long *isolated_file, unsigned long *isolated_anon)
 {
 	struct zone *zone = cc->zone;
 	unsigned long nr_scanned = 0, nr_isolated = 0;
@@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
+		if (page_is_file_cache(page))
+			(*isolated_file)++;
+		else
+			(*isolated_anon)++;
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);

Makes more sense?
-- 
Michal Hocko
SUSE Labs

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

[Minchan Kim]

On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote:
> On Fri 14-10-16 23:44:48, Minchan Kim wrote:
> > On Fri, Oct 14, 2016 at 03:53:34PM +0200, Michal Hocko wrote:
> > > On Fri 14-10-16 22:46:04, Minchan Kim wrote:
> > > [...]
> > > > > > > Why don't you simply mimic what shrink_inactive_list does? Aka count the
> > > > > > > number of isolated pages and then account them when appropriate?
> > > > > > >
> > > > > > I think i am correcting clearly wrong part. So, there is no need to
> > > > > > describe it too detailed. It's a misunderstanding, and i will add
> > > > > > more comments as you suggest.
> > > > > 
> > > > > OK, so could you explain why you prefer to relyon __PageMovable rather
> > > > > than do a trivial counting during the isolation?
> > > > 
> > > > I don't get it. Could you elaborate it a bit more?
> > > 
> > > It is really simple. You can count the number of file and anonymous
> > > pages while they are isolated and then account them to NR_ISOLATED_*
> > > later. Basically the same thing we do during the reclaim. We absolutely
> > > do not have to rely on __PageMovable and make this code more complex
> > > than necessary.
> > 
> > I don't understand your point.
> > isolate_migratepages_block can isolate any movable pages, for instance,
> > anon, file and non-lru and they are isolated into cc->migratepges.
> > Then, acct_isolated accounts them to NR_ISOLATED_*.
> > Isn't it same with the one you suggested?
> > The problem is we should identify which pages is non-lru movable first.
> > If it's not non-lru, it means the page is either anon or file so we
> > can account them. 
> > That's exactly waht Ming Ling did.
> > 
> > Sorry if I didn't get your point. Maybe, it would be better to give
> > pseudo code out of your mind for better understanding rather than
> > several ping-ping with vague words.
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 0409a4ad6ea1..6584705a46f6 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
>   */
>  static unsigned long
>  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> -			unsigned long end_pfn, isolate_mode_t isolate_mode)
> +			unsigned long end_pfn, isolate_mode_t isolate_mode,
> +			unsigned long *isolated_file, unsigned long *isolated_anon)
>  {
>  	struct zone *zone = cc->zone;
>  	unsigned long nr_scanned = 0, nr_isolated = 0;
> @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  
>  		/* Successfully isolated */
>  		del_page_from_lru_list(page, lruvec, page_lru(page));
> +		if (page_is_file_cache(page))
> +			(*isolated_file)++;
> +		else
> +			(*isolated_anon)++;
>  
>  isolate_success:
>  		list_add(&page->lru, &cc->migratepages);
> 
> Makes more sense?

It is doable for isolation part. IOW, maybe we can make acct_isolated
simple with those counters but we need to handle migrate, putback part.
If you want to remove the check of __PageMoable with those counter, it
means we should pass the counter on every functions related migration
where isolate, migrate, putback parts.

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

[Michal Hocko]

On Sat 15-10-16 00:26:33, Minchan Kim wrote:
> On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote:
[...]
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 0409a4ad6ea1..6584705a46f6 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
> >   */
> >  static unsigned long
> >  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > -			unsigned long end_pfn, isolate_mode_t isolate_mode)
> > +			unsigned long end_pfn, isolate_mode_t isolate_mode,
> > +			unsigned long *isolated_file, unsigned long *isolated_anon)
> >  {
> >  	struct zone *zone = cc->zone;
> >  	unsigned long nr_scanned = 0, nr_isolated = 0;
> > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >  
> >  		/* Successfully isolated */
> >  		del_page_from_lru_list(page, lruvec, page_lru(page));
> > +		if (page_is_file_cache(page))
> > +			(*isolated_file)++;
> > +		else
> > +			(*isolated_anon)++;
> >  
> >  isolate_success:
> >  		list_add(&page->lru, &cc->migratepages);
> > 
> > Makes more sense?
> 
> It is doable for isolation part. IOW, maybe we can make acct_isolated
> simple with those counters but we need to handle migrate, putback part.
> If you want to remove the check of __PageMoable with those counter, it
> means we should pass the counter on every functions related migration
> where isolate, migrate, putback parts.

OK, I see. Can we just get rid of acct_isolated altogether? Why cannot
we simply update NR_ISOLATED_* while isolating pages? Just looking at
isolate_migratepages_block:
			acct_isolated(zone, cc);
			putback_movable_pages(&cc->migratepages);

suggests we are doing something suboptimal. I guess we cannot get rid of
__PageMoveble checks which is sad because that just adds a lot of
confusion because checking for !__PageMovable(page) for LRU pages is
just a head scratcher (LRU pages are movable arent' they?). Maybe it
would be even good to get rid of this misnomer. PageNonLRUMovable?

Anyway, I would suggest to do something like this. Batching NR_ISOLATED*
just doesn't make all that much sense as these are per-cpu and the
resulting code seems to be easier without it.
---
diff --git a/mm/compaction.c b/mm/compaction.c
index 0409a4ad6ea1..df1fd0c20e5c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -634,22 +634,6 @@ isolate_freepages_range(struct compact_control *cc,
 	return pfn;
 }
 
-/* Update the number of anon and file isolated pages in the zone */
-static void acct_isolated(struct zone *zone, struct compact_control *cc)
-{
-	struct page *page;
-	unsigned int count[2] = { 0, };
-
-	if (list_empty(&cc->migratepages))
-		return;
-
-	list_for_each_entry(page, &cc->migratepages, lru)
-		count[!!page_is_file_cache(page)]++;
-
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
-}
-
 /* Similar to reclaim, but different enough that they don't share logic */
 static bool too_many_isolated(struct zone *zone)
 {
@@ -866,6 +850,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
+		inc_node_page_state(zone->zone_pgdat,
+				NR_ISOLATED_ANON + page_is_file_cache(page));
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);
@@ -902,7 +888,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				spin_unlock_irqrestore(zone_lru_lock(zone), flags);
 				locked = false;
 			}
-			acct_isolated(zone, cc);
 			putback_movable_pages(&cc->migratepages);
 			cc->nr_migratepages = 0;
 			cc->last_migrated_pfn = 0;
@@ -988,7 +973,6 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 		if (cc->nr_migratepages == COMPACT_CLUSTER_MAX)
 			break;
 	}
-	acct_isolated(cc->zone, cc);
 
 	return pfn;
 }
@@ -1258,10 +1242,8 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 		low_pfn = isolate_migratepages_block(cc, low_pfn,
 						block_end_pfn, isolate_mode);
 
-		if (!low_pfn || cc->contended) {
-			acct_isolated(zone, cc);
+		if (!low_pfn || cc->contended)
 			return ISOLATE_ABORT;
-		}
 
 		/*
 		 * Either we isolated something and proceed with migration. Or
@@ -1271,7 +1253,6 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 		break;
 	}
 
-	acct_isolated(zone, cc);
 	/* Record where migration scanner will be restarted. */
 	cc->migrate_pfn = low_pfn;
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 99250aee1ac1..66ce6b490b13 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -168,8 +168,6 @@ 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));
 		/*
 		 * We isolated non-lru movable page so here we can use
 		 * __PageMovable because LRU page's mapping cannot have
@@ -186,6 +184,8 @@ void putback_movable_pages(struct list_head *l)
 			put_page(page);
 		} else {
 			putback_lru_page(page);
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 		}
 	}
 }
@@ -1121,8 +1121,15 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 		 * restored.
 		 */
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+
+		/*
+		 * Compaction can migrate also non-LRU pages which are
+		 * not accounted to NR_ISOLATED_*. They can be recognized
+		 * as __PageMovable
+		 */
+		if (likely(!__PageMovable(page)))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 	}
 
 	/*

-- 
Michal Hocko
SUSE Labs

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

[Minchan Kim]

Hi Michal,

On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote:
> On Sat 15-10-16 00:26:33, Minchan Kim wrote:
> > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote:
> [...]
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index 0409a4ad6ea1..6584705a46f6 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
> > >   */
> > >  static unsigned long
> > >  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > > -			unsigned long end_pfn, isolate_mode_t isolate_mode)
> > > +			unsigned long end_pfn, isolate_mode_t isolate_mode,
> > > +			unsigned long *isolated_file, unsigned long *isolated_anon)
> > >  {
> > >  	struct zone *zone = cc->zone;
> > >  	unsigned long nr_scanned = 0, nr_isolated = 0;
> > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > >  
> > >  		/* Successfully isolated */
> > >  		del_page_from_lru_list(page, lruvec, page_lru(page));
> > > +		if (page_is_file_cache(page))
> > > +			(*isolated_file)++;
> > > +		else
> > > +			(*isolated_anon)++;
> > >  
> > >  isolate_success:
> > >  		list_add(&page->lru, &cc->migratepages);
> > > 
> > > Makes more sense?
> > 
> > It is doable for isolation part. IOW, maybe we can make acct_isolated
> > simple with those counters but we need to handle migrate, putback part.
> > If you want to remove the check of __PageMoable with those counter, it
> > means we should pass the counter on every functions related migration
> > where isolate, migrate, putback parts.
> 
> OK, I see. Can we just get rid of acct_isolated altogether? Why cannot
> we simply update NR_ISOLATED_* while isolating pages? Just looking at
> isolate_migratepages_block:
> 			acct_isolated(zone, cc);
> 			putback_movable_pages(&cc->migratepages);
> 
> suggests we are doing something suboptimal. I guess we cannot get rid of
> __PageMoveble checks which is sad because that just adds a lot of
> confusion because checking for !__PageMovable(page) for LRU pages is
> just a head scratcher (LRU pages are movable arent' they?). Maybe it
> would be even good to get rid of this misnomer. PageNonLRUMovable?

Yeah, I hated the naming but didn't have a good idea.
PageNonLRUMovable, definitely, one I thought as candidate but dropped
by lenghthy naming. If others don't object, I am happy to change it.

> 
> Anyway, I would suggest to do something like this. Batching NR_ISOLATED*
> just doesn't make all that much sense as these are per-cpu and the
> resulting code seems to be easier without it.

Agree. Could you resend it as formal patch?

> ---
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 0409a4ad6ea1..df1fd0c20e5c 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -634,22 +634,6 @@ isolate_freepages_range(struct compact_control *cc,
>  	return pfn;
>  }
>  
> -/* Update the number of anon and file isolated pages in the zone */
> -static void acct_isolated(struct zone *zone, struct compact_control *cc)
> -{
> -	struct page *page;
> -	unsigned int count[2] = { 0, };
> -
> -	if (list_empty(&cc->migratepages))
> -		return;
> -
> -	list_for_each_entry(page, &cc->migratepages, lru)
> -		count[!!page_is_file_cache(page)]++;
> -
> -	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
> -	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
> -}
> -
>  /* Similar to reclaim, but different enough that they don't share logic */
>  static bool too_many_isolated(struct zone *zone)
>  {
> @@ -866,6 +850,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  
>  		/* Successfully isolated */
>  		del_page_from_lru_list(page, lruvec, page_lru(page));
> +		inc_node_page_state(zone->zone_pgdat,
> +				NR_ISOLATED_ANON + page_is_file_cache(page));
>  
>  isolate_success:
>  		list_add(&page->lru, &cc->migratepages);
> @@ -902,7 +888,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				spin_unlock_irqrestore(zone_lru_lock(zone), flags);
>  				locked = false;
>  			}
> -			acct_isolated(zone, cc);
>  			putback_movable_pages(&cc->migratepages);
>  			cc->nr_migratepages = 0;
>  			cc->last_migrated_pfn = 0;
> @@ -988,7 +973,6 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
>  		if (cc->nr_migratepages == COMPACT_CLUSTER_MAX)
>  			break;
>  	}
> -	acct_isolated(cc->zone, cc);
>  
>  	return pfn;
>  }
> @@ -1258,10 +1242,8 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>  		low_pfn = isolate_migratepages_block(cc, low_pfn,
>  						block_end_pfn, isolate_mode);
>  
> -		if (!low_pfn || cc->contended) {
> -			acct_isolated(zone, cc);
> +		if (!low_pfn || cc->contended)
>  			return ISOLATE_ABORT;
> -		}
>  
>  		/*
>  		 * Either we isolated something and proceed with migration. Or
> @@ -1271,7 +1253,6 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>  		break;
>  	}
>  
> -	acct_isolated(zone, cc);
>  	/* Record where migration scanner will be restarted. */
>  	cc->migrate_pfn = low_pfn;
>  
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 99250aee1ac1..66ce6b490b13 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -168,8 +168,6 @@ 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));
>  		/*
>  		 * We isolated non-lru movable page so here we can use
>  		 * __PageMovable because LRU page's mapping cannot have
> @@ -186,6 +184,8 @@ void putback_movable_pages(struct list_head *l)
>  			put_page(page);
>  		} else {
>  			putback_lru_page(page);
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
>  		}
>  	}
>  }
> @@ -1121,8 +1121,15 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>  		 * restored.
>  		 */
>  		list_del(&page->lru);
> -		dec_node_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> +
> +		/*
> +		 * Compaction can migrate also non-LRU pages which are
> +		 * not accounted to NR_ISOLATED_*. They can be recognized
> +		 * as __PageMovable
> +		 */
> +		if (likely(!__PageMovable(page)))
> +			dec_node_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
>  	}
>  
>  	/*
> 
> -- 
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Mon 17-10-16 08:06:18, Minchan Kim wrote:
> Hi Michal,
> 
> On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote:
> > On Sat 15-10-16 00:26:33, Minchan Kim wrote:
> > > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote:
> > [...]
> > > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > > index 0409a4ad6ea1..6584705a46f6 100644
> > > > --- a/mm/compaction.c
> > > > +++ b/mm/compaction.c
> > > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
> > > >   */
> > > >  static unsigned long
> > > >  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > > > -			unsigned long end_pfn, isolate_mode_t isolate_mode)
> > > > +			unsigned long end_pfn, isolate_mode_t isolate_mode,
> > > > +			unsigned long *isolated_file, unsigned long *isolated_anon)
> > > >  {
> > > >  	struct zone *zone = cc->zone;
> > > >  	unsigned long nr_scanned = 0, nr_isolated = 0;
> > > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > > >  
> > > >  		/* Successfully isolated */
> > > >  		del_page_from_lru_list(page, lruvec, page_lru(page));
> > > > +		if (page_is_file_cache(page))
> > > > +			(*isolated_file)++;
> > > > +		else
> > > > +			(*isolated_anon)++;
> > > >  
> > > >  isolate_success:
> > > >  		list_add(&page->lru, &cc->migratepages);
> > > > 
> > > > Makes more sense?
> > > 
> > > It is doable for isolation part. IOW, maybe we can make acct_isolated
> > > simple with those counters but we need to handle migrate, putback part.
> > > If you want to remove the check of __PageMoable with those counter, it
> > > means we should pass the counter on every functions related migration
> > > where isolate, migrate, putback parts.
> > 
> > OK, I see. Can we just get rid of acct_isolated altogether? Why cannot
> > we simply update NR_ISOLATED_* while isolating pages? Just looking at
> > isolate_migratepages_block:
> > 			acct_isolated(zone, cc);
> > 			putback_movable_pages(&cc->migratepages);
> > 
> > suggests we are doing something suboptimal. I guess we cannot get rid of
> > __PageMoveble checks which is sad because that just adds a lot of
> > confusion because checking for !__PageMovable(page) for LRU pages is
> > just a head scratcher (LRU pages are movable arent' they?). Maybe it
> > would be even good to get rid of this misnomer. PageNonLRUMovable?
> 
> Yeah, I hated the naming but didn't have a good idea.
> PageNonLRUMovable, definitely, one I thought as candidate but dropped
> by lenghthy naming. If others don't object, I am happy to change it.

Yes it is long but it is less confusing because it is just utterly
confusing to test for LRU pages with !__PageMovable when in fact they
are movable. Heck even unreclaimable pages are movable unless explicitly
configured to not be.
 
> > Anyway, I would suggest to do something like this. Batching NR_ISOLATED*
> > just doesn't make all that much sense as these are per-cpu and the
> > resulting code seems to be easier without it.
> 
> Agree. Could you resend it as formal patch?

Sure, what do you think about the following? I haven't marked it for
stable because there was no bug report for it AFAIU.
---
>From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001
From: Ming Ling <ming.ling@spreadtrum.com>
Date: Mon, 17 Oct 2016 10:26:50 +0200
Subject: [PATCH] mm, compaction: fix NR_ISOLATED_* stats for pfn based
 migration

Since bda807d44454 ("mm: migrate: support non-lru movable page
migration") isolate_migratepages_block) can isolate !PageLRU pages which
would acct_isolated account as NR_ISOLATED_*. Accounting these non-lru
pages NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
heuristics based on those counters such as pgdat_reclaimable_pages resp.
too_many_isolated which would lead to unexpected stalls during the
direct reclaim without any good reason. Note that
__alloc_contig_migrate_range can isolate a lot of pages at once.

On mobile devices such as 512M ram android Phone, it may use a big zram
swap. In some cases zram(zsmalloc) uses too many non-lru but migratedable
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

Fix this by only accounting lru pages to NR_ISOLATED_* in
isolate_migratepages_block right after they were isolated and we still
know they were on LRU. Drop acct_isolated because it is called after the
fact and we've lost that information. Batching per-cpu counter doesn't
make much improvement anyway. Also make sure that we uncharge only LRU
pages when putting them back on the LRU in putback_movable_pages resp.
when unmap_and_move migrates the page.

Fixes: bda807d44454 ("mm: migrate: support non-lru movable page migration")
Signed-off-by: Ming Ling <ming.ling@spreadtrum.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/compaction.c | 25 +++----------------------
 mm/migrate.c    | 15 +++++++++++----
 2 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 0409a4ad6ea1..df1fd0c20e5c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -634,22 +634,6 @@ isolate_freepages_range(struct compact_control *cc,
 	return pfn;
 }
 
-/* Update the number of anon and file isolated pages in the zone */
-static void acct_isolated(struct zone *zone, struct compact_control *cc)
-{
-	struct page *page;
-	unsigned int count[2] = { 0, };
-
-	if (list_empty(&cc->migratepages))
-		return;
-
-	list_for_each_entry(page, &cc->migratepages, lru)
-		count[!!page_is_file_cache(page)]++;
-
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]);
-	mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]);
-}
-
 /* Similar to reclaim, but different enough that they don't share logic */
 static bool too_many_isolated(struct zone *zone)
 {
@@ -866,6 +850,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
+		inc_node_page_state(zone->zone_pgdat,
+				NR_ISOLATED_ANON + page_is_file_cache(page));
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);
@@ -902,7 +888,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 				spin_unlock_irqrestore(zone_lru_lock(zone), flags);
 				locked = false;
 			}
-			acct_isolated(zone, cc);
 			putback_movable_pages(&cc->migratepages);
 			cc->nr_migratepages = 0;
 			cc->last_migrated_pfn = 0;
@@ -988,7 +973,6 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 		if (cc->nr_migratepages == COMPACT_CLUSTER_MAX)
 			break;
 	}
-	acct_isolated(cc->zone, cc);
 
 	return pfn;
 }
@@ -1258,10 +1242,8 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 		low_pfn = isolate_migratepages_block(cc, low_pfn,
 						block_end_pfn, isolate_mode);
 
-		if (!low_pfn || cc->contended) {
-			acct_isolated(zone, cc);
+		if (!low_pfn || cc->contended)
 			return ISOLATE_ABORT;
-		}
 
 		/*
 		 * Either we isolated something and proceed with migration. Or
@@ -1271,7 +1253,6 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 		break;
 	}
 
-	acct_isolated(zone, cc);
 	/* Record where migration scanner will be restarted. */
 	cc->migrate_pfn = low_pfn;
 
diff --git a/mm/migrate.c b/mm/migrate.c
index 99250aee1ac1..66ce6b490b13 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -168,8 +168,6 @@ 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));
 		/*
 		 * We isolated non-lru movable page so here we can use
 		 * __PageMovable because LRU page's mapping cannot have
@@ -186,6 +184,8 @@ void putback_movable_pages(struct list_head *l)
 			put_page(page);
 		} else {
 			putback_lru_page(page);
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 		}
 	}
 }
@@ -1121,8 +1121,15 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
 		 * restored.
 		 */
 		list_del(&page->lru);
-		dec_node_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
+
+		/*
+		 * Compaction can migrate also non-LRU pages which are
+		 * not accounted to NR_ISOLATED_*. They can be recognized
+		 * as __PageMovable
+		 */
+		if (likely(!__PageMovable(page)))
+			dec_node_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
 	}
 
 	/*
-- 
2.9.3


-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Mon 17-10-16 10:42:44, Michal Hocko wrote:
[...]
> Sure, what do you think about the following? I haven't marked it for
> stable because there was no bug report for it AFAIU.

And 0-day robot just noticed that I've screwed and need the following on
top. If the patch makes sense I will repost it to Andrew with this
folded in.
---
diff --git a/mm/compaction.c b/mm/compaction.c
index df1fd0c20e5c..70e6bec46dc2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -850,7 +850,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 		/* Successfully isolated */
 		del_page_from_lru_list(page, lruvec, page_lru(page));
-		inc_node_page_state(zone->zone_pgdat,
+		inc_node_page_state(page,
 				NR_ISOLATED_ANON + page_is_file_cache(page));
 
 isolate_success:

-- 
Michal Hocko
SUSE Labs

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

[Minchan Kim]

On Mon, Oct 17, 2016 at 10:42:45AM +0200, Michal Hocko wrote:
> On Mon 17-10-16 08:06:18, Minchan Kim wrote:
> > Hi Michal,
> > 
> > On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote:
> > > On Sat 15-10-16 00:26:33, Minchan Kim wrote:
> > > > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > > > index 0409a4ad6ea1..6584705a46f6 100644
> > > > > --- a/mm/compaction.c
> > > > > +++ b/mm/compaction.c
> > > > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone)
> > > > >   */
> > > > >  static unsigned long
> > > > >  isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > > > > -			unsigned long end_pfn, isolate_mode_t isolate_mode)
> > > > > +			unsigned long end_pfn, isolate_mode_t isolate_mode,
> > > > > +			unsigned long *isolated_file, unsigned long *isolated_anon)
> > > > >  {
> > > > >  	struct zone *zone = cc->zone;
> > > > >  	unsigned long nr_scanned = 0, nr_isolated = 0;
> > > > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> > > > >  
> > > > >  		/* Successfully isolated */
> > > > >  		del_page_from_lru_list(page, lruvec, page_lru(page));
> > > > > +		if (page_is_file_cache(page))
> > > > > +			(*isolated_file)++;
> > > > > +		else
> > > > > +			(*isolated_anon)++;
> > > > >  
> > > > >  isolate_success:
> > > > >  		list_add(&page->lru, &cc->migratepages);
> > > > > 
> > > > > Makes more sense?
> > > > 
> > > > It is doable for isolation part. IOW, maybe we can make acct_isolated
> > > > simple with those counters but we need to handle migrate, putback part.
> > > > If you want to remove the check of __PageMoable with those counter, it
> > > > means we should pass the counter on every functions related migration
> > > > where isolate, migrate, putback parts.
> > > 
> > > OK, I see. Can we just get rid of acct_isolated altogether? Why cannot
> > > we simply update NR_ISOLATED_* while isolating pages? Just looking at
> > > isolate_migratepages_block:
> > > 			acct_isolated(zone, cc);
> > > 			putback_movable_pages(&cc->migratepages);
> > > 
> > > suggests we are doing something suboptimal. I guess we cannot get rid of
> > > __PageMoveble checks which is sad because that just adds a lot of
> > > confusion because checking for !__PageMovable(page) for LRU pages is
> > > just a head scratcher (LRU pages are movable arent' they?). Maybe it
> > > would be even good to get rid of this misnomer. PageNonLRUMovable?
> > 
> > Yeah, I hated the naming but didn't have a good idea.
> > PageNonLRUMovable, definitely, one I thought as candidate but dropped
> > by lenghthy naming. If others don't object, I am happy to change it.
> 
> Yes it is long but it is less confusing because it is just utterly
> confusing to test for LRU pages with !__PageMovable when in fact they
> are movable. Heck even unreclaimable pages are movable unless explicitly
> configured to not be.
>  
> > > Anyway, I would suggest to do something like this. Batching NR_ISOLATED*
> > > just doesn't make all that much sense as these are per-cpu and the
> > > resulting code seems to be easier without it.
> > 
> > Agree. Could you resend it as formal patch?
> 
> Sure, what do you think about the following? I haven't marked it for
> stable because there was no bug report for it AFAIU.
> ---
> From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001
> From: Ming Ling <ming.ling@spreadtrum.com>
> Date: Mon, 17 Oct 2016 10:26:50 +0200
> Subject: [PATCH] mm, compaction: fix NR_ISOLATED_* stats for pfn based
>  migration
> 
> Since bda807d44454 ("mm: migrate: support non-lru movable page
> migration") isolate_migratepages_block) can isolate !PageLRU pages which
> would acct_isolated account as NR_ISOLATED_*. Accounting these non-lru
> pages NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
> heuristics based on those counters such as pgdat_reclaimable_pages resp.
> too_many_isolated which would lead to unexpected stalls during the
> direct reclaim without any good reason. Note that
> __alloc_contig_migrate_range can isolate a lot of pages at once.
> 
> On mobile devices such as 512M ram android Phone, it may use a big zram
> swap. In some cases zram(zsmalloc) uses too many non-lru but migratedable
> 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
> 
> Fix this by only accounting lru pages to NR_ISOLATED_* in
> isolate_migratepages_block right after they were isolated and we still
> know they were on LRU. Drop acct_isolated because it is called after the
> fact and we've lost that information. Batching per-cpu counter doesn't
> make much improvement anyway. Also make sure that we uncharge only LRU
> pages when putting them back on the LRU in putback_movable_pages resp.
> when unmap_and_move migrates the page.
> 
> Fixes: bda807d44454 ("mm: migrate: support non-lru movable page migration")
> Signed-off-by: Ming Ling <ming.ling@spreadtrum.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Minchan Kim <minchan@kernel.org>

with folding other fix patch you posted.

Thanks.

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

[Michal Hocko]

On Tue 18-10-16 15:29:50, Minchan Kim wrote:
> On Mon, Oct 17, 2016 at 10:42:45AM +0200, Michal Hocko wrote:
[...]
> > Sure, what do you think about the following? I haven't marked it for
> > stable because there was no bug report for it AFAIU.
> > ---
> > From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001
> > From: Ming Ling <ming.ling@spreadtrum.com>
> > Date: Mon, 17 Oct 2016 10:26:50 +0200
> > Subject: [PATCH] mm, compaction: fix NR_ISOLATED_* stats for pfn based
> >  migration
> > 
> > Since bda807d44454 ("mm: migrate: support non-lru movable page
> > migration") isolate_migratepages_block) can isolate !PageLRU pages which
> > would acct_isolated account as NR_ISOLATED_*. Accounting these non-lru
> > pages NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
> > heuristics based on those counters such as pgdat_reclaimable_pages resp.
> > too_many_isolated which would lead to unexpected stalls during the
> > direct reclaim without any good reason. Note that
> > __alloc_contig_migrate_range can isolate a lot of pages at once.
> > 
> > On mobile devices such as 512M ram android Phone, it may use a big zram
> > swap. In some cases zram(zsmalloc) uses too many non-lru but migratedable
> > 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
> > 
> > Fix this by only accounting lru pages to NR_ISOLATED_* in
> > isolate_migratepages_block right after they were isolated and we still
> > know they were on LRU. Drop acct_isolated because it is called after the
> > fact and we've lost that information. Batching per-cpu counter doesn't
> > make much improvement anyway. Also make sure that we uncharge only LRU
> > pages when putting them back on the LRU in putback_movable_pages resp.
> > when unmap_and_move migrates the page.
> > 
> > Fixes: bda807d44454 ("mm: migrate: support non-lru movable page migration")
> > Signed-off-by: Ming Ling <ming.ling@spreadtrum.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> with folding other fix patch you posted.

Thanks.

Ming, are you OK with this patch? Can I post it to Andrew?
-- 
Michal Hocko
SUSE Labs

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

[Ming Ling]

二, 10月 18, 2016 at 02:52:47下午 +0200, Michal Hocko wrote:
hi,
> On Tue 18-10-16 15:29:50, Minchan Kim wrote:
> > On Mon, Oct 17, 2016 at 10:42:45AM +0200, Michal Hocko wrote:
> [...]
> > > Sure, what do you think about the following? I haven't marked it for
> > > stable because there was no bug report for it AFAIU.
> > > ---
> > > From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001
> > > From: Ming Ling <ming.ling@spreadtrum.com>
> > > Date: Mon, 17 Oct 2016 10:26:50 +0200
> > > Subject: [PATCH] mm, compaction: fix NR_ISOLATED_* stats for pfn based
> > >  migration
> > > 
> > > Since bda807d44454 ("mm: migrate: support non-lru movable page
> > > migration") isolate_migratepages_block) can isolate !PageLRU pages which
> > > would acct_isolated account as NR_ISOLATED_*. Accounting these non-lru
> > > pages NR_ISOLATED_{ANON,FILE} doesn't make any sense and it can misguide
> > > heuristics based on those counters such as pgdat_reclaimable_pages resp.
> > > too_many_isolated which would lead to unexpected stalls during the
> > > direct reclaim without any good reason. Note that
> > > __alloc_contig_migrate_range can isolate a lot of pages at once.
> > > 
> > > On mobile devices such as 512M ram android Phone, it may use a big zram
> > > swap. In some cases zram(zsmalloc) uses too many non-lru but migratedable
> > > 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
> > > 
> > > Fix this by only accounting lru pages to NR_ISOLATED_* in
> > > isolate_migratepages_block right after they were isolated and we still
> > > know they were on LRU. Drop acct_isolated because it is called after the
> > > fact and we've lost that information. Batching per-cpu counter doesn't
> > > make much improvement anyway. Also make sure that we uncharge only LRU
> > > pages when putting them back on the LRU in putback_movable_pages resp.
> > > when unmap_and_move migrates the page.
> > > 
> > > Fixes: bda807d44454 ("mm: migrate: support non-lru movable page migration")
> > > Signed-off-by: Ming Ling <ming.ling@spreadtrum.com>
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > Acked-by: Minchan Kim <minchan@kernel.org>
> > 
> > with folding other fix patch you posted.
> 
> Thanks.
> 
> Ming, are you OK with this patch? Can I post it to Andrew?
> --
I think that's fine. Just do it.
Thank you.
> Michal Hocko
> SUSE Labs