From: Ming Ling <[email protected]>
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 <[email protected]>
---
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
On Thu 13-10-16 14:39:09, ming.ling wrote:
> From: Ming Ling <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > >
> > > 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
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
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
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.
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
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.
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
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Michal Hocko <[email protected]>
---
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
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
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Michal Hocko <[email protected]>
Acked-by: Minchan Kim <[email protected]>
with folding other fix patch you posted.
Thanks.
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 <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Michal Hocko <[email protected]>
>
> Acked-by: Minchan Kim <[email protected]>
>
> 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
二, 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 <[email protected]>
> > > 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 <[email protected]>
> > > Signed-off-by: Michal Hocko <[email protected]>
> >
> > Acked-by: Minchan Kim <[email protected]>
> >
> > 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