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 
> > > 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 
> > > Signed-off-by: Michal Hocko 
> > 
> > Acked-by: Minchan Kim 
> > 
> > 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

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 
> > > 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 
> > > Signed-off-by: Michal Hocko 
> > 
> > Acked-by: Minchan Kim 
> > 
> > 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

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 
> > 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 
> > Signed-off-by: Michal Hocko 
> 
> Acked-by: Minchan Kim 
> 
> 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.

[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 
> > 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 
> > Signed-off-by: Michal Hocko 
> 
> Acked-by: Minchan Kim 
> 
> 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.

[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(>lru, >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(>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 
> 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 

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(>lru, >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(>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 
> 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 

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.

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

[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(>lru, >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(>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 
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 

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(>lru, >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(>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 
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 

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(>lru, >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(>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(>migratepages))
> - return;
> -
> - list_for_each_entry(page, >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(>lru, >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(>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 

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(>lru, >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(>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(>migratepages))
> - return;
> -
> - list_for_each_entry(page, >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(>lru, >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(>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 

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(>lru, >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(>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(>migratepages))
-   return;
-
-   list_for_each_entry(page, >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(>lru, >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(>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 

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(>lru, >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(>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(>migratepages))
-   return;
-
-   list_for_each_entry(page, >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(>lru, >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(>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 

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(>lru, >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.

[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(>lru, >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 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(>lru, >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.

[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(>lru, >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 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.

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

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

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.

[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 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 
> > > 
> > > 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);
> 

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

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

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

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 
> 
> 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 
> ---
>  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(>migratepages))
>   return;
>  
> - list_for_each_entry(page, >migratepages, lru)
> - count[!!page_is_file_cache(page)]++;
> + list_for_each_entry(page, >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(>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(>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 Thu 13-10-16 14:39:09, ming.ling wrote:
> From: Ming Ling 
> 
> 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 
> ---
>  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(>migratepages))
>   return;
>  
> - list_for_each_entry(page, >migratepages, lru)
> - count[!!page_is_file_cache(page)]++;
> + list_for_each_entry(page, >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(>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(>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

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

[ming.ling]

From: Ming Ling 

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 
---
 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(>migratepages))
return;
 
-   list_for_each_entry(page, >migratepages, lru)
-   count[!!page_is_file_cache(page)]++;
+   list_for_each_entry(page, >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(>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(>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

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

[ming.ling]

From: Ming Ling 

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 
---
 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(>migratepages))
return;
 
-   list_for_each_entry(page, >migratepages, lru)
-   count[!!page_is_file_cache(page)]++;
+   list_for_each_entry(page, >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(>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(>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