Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
二, 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.
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.
On Mon, Oct 17, 2016 at 10:42:45AM +0200, Michal Hocko wrote: > On Mon 17-10-16 08:06:18, Minchan Kim wrote: > > Hi Michal, > > > > On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote: > > > On Sat 15-10-16 00:26:33, Minchan Kim wrote: > > > > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote: > > > [...] > > > > > diff --git a/mm/compaction.c b/mm/compaction.c > > > > > index 0409a4ad6ea1..6584705a46f6 100644 > > > > > --- a/mm/compaction.c > > > > > +++ b/mm/compaction.c > > > > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) > > > > > */ > > > > > static unsigned long > > > > > isolate_migratepages_block(struct compact_control *cc, unsigned long > > > > > low_pfn, > > > > > - unsigned long end_pfn, isolate_mode_t > > > > > isolate_mode) > > > > > + unsigned long end_pfn, isolate_mode_t > > > > > isolate_mode, > > > > > + unsigned long *isolated_file, unsigned long > > > > > *isolated_anon) > > > > > { > > > > > struct zone *zone = cc->zone; > > > > > unsigned long nr_scanned = 0, nr_isolated = 0; > > > > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct > > > > > compact_control *cc, unsigned long low_pfn, > > > > > > > > > > /* Successfully isolated */ > > > > > del_page_from_lru_list(page, lruvec, page_lru(page)); > > > > > + if (page_is_file_cache(page)) > > > > > + (*isolated_file)++; > > > > > + else > > > > > + (*isolated_anon)++; > > > > > > > > > > isolate_success: > > > > > list_add(&page->lru, &cc->migratepages); > > > > > > > > > > Makes more sense? > > > > > > > > It is doable for isolation part. IOW, maybe we can make acct_isolated > > > > simple with those counters but we need to handle migrate, putback part. > > > > If you want to remove the check of __PageMoable with those counter, it > > > > means we should pass the counter on every functions related migration > > > > where isolate, migrate, putback parts. > > > > > > OK, I see. Can we just get rid of acct_isolated altogether? Why cannot > > > we simply update NR_ISOLATED_* while isolating pages? Just looking at > > > isolate_migratepages_block: > > > acct_isolated(zone, cc); > > > putback_movable_pages(&cc->migratepages); > > > > > > suggests we are doing something suboptimal. I guess we cannot get rid of > > > __PageMoveble checks which is sad because that just adds a lot of > > > confusion because checking for !__PageMovable(page) for LRU pages is > > > just a head scratcher (LRU pages are movable arent' they?). Maybe it > > > would be even good to get rid of this misnomer. PageNonLRUMovable? > > > > Yeah, I hated the naming but didn't have a good idea. > > PageNonLRUMovable, definitely, one I thought as candidate but dropped > > by lenghthy naming. If others don't object, I am happy to change it. > > Yes it is long but it is less confusing because it is just utterly > confusing to test for LRU pages with !__PageMovable when in fact they > are movable. Heck even unreclaimable pages are movable unless explicitly > configured to not be. > > > > Anyway, I would suggest to do something like this. Batching NR_ISOLATED* > > > just doesn't make all that much sense as these are per-cpu and the > > > resulting code seems to be easier without it. > > > > Agree. Could you resend it as formal patch? > > Sure, what do you think about the following? I haven't marked it for > stable because there was no bug report for it AFAIU. > --- > From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001 > From: Ming Ling > 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 > k
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
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.
On Mon 17-10-16 08:06:18, Minchan Kim wrote: > Hi Michal, > > On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote: > > On Sat 15-10-16 00:26:33, Minchan Kim wrote: > > > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote: > > [...] > > > > diff --git a/mm/compaction.c b/mm/compaction.c > > > > index 0409a4ad6ea1..6584705a46f6 100644 > > > > --- a/mm/compaction.c > > > > +++ b/mm/compaction.c > > > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) > > > > */ > > > > static unsigned long > > > > isolate_migratepages_block(struct compact_control *cc, unsigned long > > > > low_pfn, > > > > - unsigned long end_pfn, isolate_mode_t > > > > isolate_mode) > > > > + unsigned long end_pfn, isolate_mode_t > > > > isolate_mode, > > > > + unsigned long *isolated_file, unsigned long > > > > *isolated_anon) > > > > { > > > > struct zone *zone = cc->zone; > > > > unsigned long nr_scanned = 0, nr_isolated = 0; > > > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control > > > > *cc, unsigned long low_pfn, > > > > > > > > /* Successfully isolated */ > > > > del_page_from_lru_list(page, lruvec, page_lru(page)); > > > > + if (page_is_file_cache(page)) > > > > + (*isolated_file)++; > > > > + else > > > > + (*isolated_anon)++; > > > > > > > > isolate_success: > > > > list_add(&page->lru, &cc->migratepages); > > > > > > > > Makes more sense? > > > > > > It is doable for isolation part. IOW, maybe we can make acct_isolated > > > simple with those counters but we need to handle migrate, putback part. > > > If you want to remove the check of __PageMoable with those counter, it > > > means we should pass the counter on every functions related migration > > > where isolate, migrate, putback parts. > > > > OK, I see. Can we just get rid of acct_isolated altogether? Why cannot > > we simply update NR_ISOLATED_* while isolating pages? Just looking at > > isolate_migratepages_block: > > acct_isolated(zone, cc); > > putback_movable_pages(&cc->migratepages); > > > > suggests we are doing something suboptimal. I guess we cannot get rid of > > __PageMoveble checks which is sad because that just adds a lot of > > confusion because checking for !__PageMovable(page) for LRU pages is > > just a head scratcher (LRU pages are movable arent' they?). Maybe it > > would be even good to get rid of this misnomer. PageNonLRUMovable? > > Yeah, I hated the naming but didn't have a good idea. > PageNonLRUMovable, definitely, one I thought as candidate but dropped > by lenghthy naming. If others don't object, I am happy to change it. Yes it is long but it is less confusing because it is just utterly confusing to test for LRU pages with !__PageMovable when in fact they are movable. Heck even unreclaimable pages are movable unless explicitly configured to not be. > > Anyway, I would suggest to do something like this. Batching NR_ISOLATED* > > just doesn't make all that much sense as these are per-cpu and the > > resulting code seems to be easier without it. > > Agree. Could you resend it as formal patch? Sure, what do you think about the following? I haven't marked it for stable because there was no bug report for it AFAIU. --- >From 3b2bd4486f36ada9f6dc86d3946855281455ba9f Mon Sep 17 00:00:00 2001 From: Ming Ling 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
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
Hi Michal, On Sat, Oct 15, 2016 at 09:10:45AM +0200, Michal Hocko wrote: > On Sat 15-10-16 00:26:33, Minchan Kim wrote: > > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote: > [...] > > > diff --git a/mm/compaction.c b/mm/compaction.c > > > index 0409a4ad6ea1..6584705a46f6 100644 > > > --- a/mm/compaction.c > > > +++ b/mm/compaction.c > > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) > > > */ > > > static unsigned long > > > isolate_migratepages_block(struct compact_control *cc, unsigned long > > > low_pfn, > > > - unsigned long end_pfn, isolate_mode_t isolate_mode) > > > + unsigned long end_pfn, isolate_mode_t isolate_mode, > > > + unsigned long *isolated_file, unsigned long > > > *isolated_anon) > > > { > > > struct zone *zone = cc->zone; > > > unsigned long nr_scanned = 0, nr_isolated = 0; > > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control > > > *cc, unsigned long low_pfn, > > > > > > /* Successfully isolated */ > > > del_page_from_lru_list(page, lruvec, page_lru(page)); > > > + if (page_is_file_cache(page)) > > > + (*isolated_file)++; > > > + else > > > + (*isolated_anon)++; > > > > > > isolate_success: > > > list_add(&page->lru, &cc->migratepages); > > > > > > Makes more sense? > > > > It is doable for isolation part. IOW, maybe we can make acct_isolated > > simple with those counters but we need to handle migrate, putback part. > > If you want to remove the check of __PageMoable with those counter, it > > means we should pass the counter on every functions related migration > > where isolate, migrate, putback parts. > > OK, I see. Can we just get rid of acct_isolated altogether? Why cannot > we simply update NR_ISOLATED_* while isolating pages? Just looking at > isolate_migratepages_block: > acct_isolated(zone, cc); > putback_movable_pages(&cc->migratepages); > > suggests we are doing something suboptimal. I guess we cannot get rid of > __PageMoveble checks which is sad because that just adds a lot of > confusion because checking for !__PageMovable(page) for LRU pages is > just a head scratcher (LRU pages are movable arent' they?). Maybe it > would be even good to get rid of this misnomer. PageNonLRUMovable? Yeah, I hated the naming but didn't have a good idea. PageNonLRUMovable, definitely, one I thought as candidate but dropped by lenghthy naming. If others don't object, I am happy to change it. > > Anyway, I would suggest to do something like this. Batching NR_ISOLATED* > just doesn't make all that much sense as these are per-cpu and the > resulting code seems to be easier without it. Agree. Could you resend it as formal patch? > --- > diff --git a/mm/compaction.c b/mm/compaction.c > index 0409a4ad6ea1..df1fd0c20e5c 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -634,22 +634,6 @@ isolate_freepages_range(struct compact_control *cc, > return pfn; > } > > -/* Update the number of anon and file isolated pages in the zone */ > -static void acct_isolated(struct zone *zone, struct compact_control *cc) > -{ > - struct page *page; > - unsigned int count[2] = { 0, }; > - > - if (list_empty(&cc->migratepages)) > - return; > - > - list_for_each_entry(page, &cc->migratepages, lru) > - count[!!page_is_file_cache(page)]++; > - > - mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]); > - mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]); > -} > - > /* Similar to reclaim, but different enough that they don't share logic */ > static bool too_many_isolated(struct zone *zone) > { > @@ -866,6 +850,8 @@ isolate_migratepages_block(struct compact_control *cc, > unsigned long low_pfn, > > /* Successfully isolated */ > del_page_from_lru_list(page, lruvec, page_lru(page)); > + inc_node_page_state(zone->zone_pgdat, > + NR_ISOLATED_ANON + page_is_file_cache(page)); > > isolate_success: > list_add(&page->lru, &cc->migratepages); > @@ -902,7 +888,6 @@ isolate_migratepages_block(struct compact_control *cc, > unsigned long low_pfn, > spin_unlock_irqrestore(zone_lru_lock(zone), > flags); > locked = false; > } > - acct_isolated(zone, cc); > putback_movable_pages(&cc->migratepages); > cc->nr_migratepages = 0; > cc->last_migrated_pfn = 0; > @@ -988,7 +973,6 @@ isolate_migratepages_range(struct compact_control *cc, > unsigned long start_pfn, > if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) > break; > } > - acct_isolated(cc->zone, cc); > > return pfn; > } > @@ -1258,10 +
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
On Sat 15-10-16 00:26:33, Minchan Kim wrote: > On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote: [...] > > diff --git a/mm/compaction.c b/mm/compaction.c > > index 0409a4ad6ea1..6584705a46f6 100644 > > --- a/mm/compaction.c > > +++ b/mm/compaction.c > > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) > > */ > > static unsigned long > > isolate_migratepages_block(struct compact_control *cc, unsigned long > > low_pfn, > > - unsigned long end_pfn, isolate_mode_t isolate_mode) > > + unsigned long end_pfn, isolate_mode_t isolate_mode, > > + unsigned long *isolated_file, unsigned long > > *isolated_anon) > > { > > struct zone *zone = cc->zone; > > unsigned long nr_scanned = 0, nr_isolated = 0; > > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, > > unsigned long low_pfn, > > > > /* Successfully isolated */ > > del_page_from_lru_list(page, lruvec, page_lru(page)); > > + if (page_is_file_cache(page)) > > + (*isolated_file)++; > > + else > > + (*isolated_anon)++; > > > > isolate_success: > > list_add(&page->lru, &cc->migratepages); > > > > Makes more sense? > > It is doable for isolation part. IOW, maybe we can make acct_isolated > simple with those counters but we need to handle migrate, putback part. > If you want to remove the check of __PageMoable with those counter, it > means we should pass the counter on every functions related migration > where isolate, migrate, putback parts. OK, I see. Can we just get rid of acct_isolated altogether? Why cannot we simply update NR_ISOLATED_* while isolating pages? Just looking at isolate_migratepages_block: acct_isolated(zone, cc); putback_movable_pages(&cc->migratepages); suggests we are doing something suboptimal. I guess we cannot get rid of __PageMoveble checks which is sad because that just adds a lot of confusion because checking for !__PageMovable(page) for LRU pages is just a head scratcher (LRU pages are movable arent' they?). Maybe it would be even good to get rid of this misnomer. PageNonLRUMovable? Anyway, I would suggest to do something like this. Batching NR_ISOLATED* just doesn't make all that much sense as these are per-cpu and the resulting code seems to be easier without it. --- diff --git a/mm/compaction.c b/mm/compaction.c index 0409a4ad6ea1..df1fd0c20e5c 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -634,22 +634,6 @@ isolate_freepages_range(struct compact_control *cc, return pfn; } -/* Update the number of anon and file isolated pages in the zone */ -static void acct_isolated(struct zone *zone, struct compact_control *cc) -{ - struct page *page; - unsigned int count[2] = { 0, }; - - if (list_empty(&cc->migratepages)) - return; - - list_for_each_entry(page, &cc->migratepages, lru) - count[!!page_is_file_cache(page)]++; - - mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]); - mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]); -} - /* Similar to reclaim, but different enough that they don't share logic */ static bool too_many_isolated(struct zone *zone) { @@ -866,6 +850,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, /* Successfully isolated */ del_page_from_lru_list(page, lruvec, page_lru(page)); + inc_node_page_state(zone->zone_pgdat, + NR_ISOLATED_ANON + page_is_file_cache(page)); isolate_success: list_add(&page->lru, &cc->migratepages); @@ -902,7 +888,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, spin_unlock_irqrestore(zone_lru_lock(zone), flags); locked = false; } - acct_isolated(zone, cc); putback_movable_pages(&cc->migratepages); cc->nr_migratepages = 0; cc->last_migrated_pfn = 0; @@ -988,7 +973,6 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn, if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) break; } - acct_isolated(cc->zone, cc); return pfn; } @@ -1258,10 +1242,8 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone, low_pfn = isolate_migratepages_block(cc, low_pfn, block_end_pfn, isolate_mode); - if (!low_pfn || cc->contended) { - acct_isolated(zone, cc); + if (!low_pfn || cc->contended) return ISOLATE_ABORT; - } /*
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
On Fri, Oct 14, 2016 at 05:03:55PM +0200, Michal Hocko wrote: > On Fri 14-10-16 23:44:48, Minchan Kim wrote: > > On Fri, Oct 14, 2016 at 03:53:34PM +0200, Michal Hocko wrote: > > > On Fri 14-10-16 22:46:04, Minchan Kim wrote: > > > [...] > > > > > > > Why don't you simply mimic what shrink_inactive_list does? Aka > > > > > > > count the > > > > > > > number of isolated pages and then account them when appropriate? > > > > > > > > > > > > > I think i am correcting clearly wrong part. So, there is no need to > > > > > > describe it too detailed. It's a misunderstanding, and i will add > > > > > > more comments as you suggest. > > > > > > > > > > OK, so could you explain why you prefer to relyon __PageMovable rather > > > > > than do a trivial counting during the isolation? > > > > > > > > I don't get it. Could you elaborate it a bit more? > > > > > > It is really simple. You can count the number of file and anonymous > > > pages while they are isolated and then account them to NR_ISOLATED_* > > > later. Basically the same thing we do during the reclaim. We absolutely > > > do not have to rely on __PageMovable and make this code more complex > > > than necessary. > > > > I don't understand your point. > > isolate_migratepages_block can isolate any movable pages, for instance, > > anon, file and non-lru and they are isolated into cc->migratepges. > > Then, acct_isolated accounts them to NR_ISOLATED_*. > > Isn't it same with the one you suggested? > > The problem is we should identify which pages is non-lru movable first. > > If it's not non-lru, it means the page is either anon or file so we > > can account them. > > That's exactly waht Ming Ling did. > > > > Sorry if I didn't get your point. Maybe, it would be better to give > > pseudo code out of your mind for better understanding rather than > > several ping-ping with vague words. > > diff --git a/mm/compaction.c b/mm/compaction.c > index 0409a4ad6ea1..6584705a46f6 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) > */ > static unsigned long > isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, > - unsigned long end_pfn, isolate_mode_t isolate_mode) > + unsigned long end_pfn, isolate_mode_t isolate_mode, > + unsigned long *isolated_file, unsigned long > *isolated_anon) > { > struct zone *zone = cc->zone; > unsigned long nr_scanned = 0, nr_isolated = 0; > @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, > unsigned long low_pfn, > > /* Successfully isolated */ > del_page_from_lru_list(page, lruvec, page_lru(page)); > + if (page_is_file_cache(page)) > + (*isolated_file)++; > + else > + (*isolated_anon)++; > > isolate_success: > list_add(&page->lru, &cc->migratepages); > > Makes more sense? It is doable for isolation part. IOW, maybe we can make acct_isolated simple with those counters but we need to handle migrate, putback part. If you want to remove the check of __PageMoable with those counter, it means we should pass the counter on every functions related migration where isolate, migrate, putback parts.
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
On Fri 14-10-16 23:44:48, Minchan Kim wrote: > On Fri, Oct 14, 2016 at 03:53:34PM +0200, Michal Hocko wrote: > > On Fri 14-10-16 22:46:04, Minchan Kim wrote: > > [...] > > > > > > Why don't you simply mimic what shrink_inactive_list does? Aka > > > > > > count the > > > > > > number of isolated pages and then account them when appropriate? > > > > > > > > > > > I think i am correcting clearly wrong part. So, there is no need to > > > > > describe it too detailed. It's a misunderstanding, and i will add > > > > > more comments as you suggest. > > > > > > > > OK, so could you explain why you prefer to relyon __PageMovable rather > > > > than do a trivial counting during the isolation? > > > > > > I don't get it. Could you elaborate it a bit more? > > > > It is really simple. You can count the number of file and anonymous > > pages while they are isolated and then account them to NR_ISOLATED_* > > later. Basically the same thing we do during the reclaim. We absolutely > > do not have to rely on __PageMovable and make this code more complex > > than necessary. > > I don't understand your point. > isolate_migratepages_block can isolate any movable pages, for instance, > anon, file and non-lru and they are isolated into cc->migratepges. > Then, acct_isolated accounts them to NR_ISOLATED_*. > Isn't it same with the one you suggested? > The problem is we should identify which pages is non-lru movable first. > If it's not non-lru, it means the page is either anon or file so we > can account them. > That's exactly waht Ming Ling did. > > Sorry if I didn't get your point. Maybe, it would be better to give > pseudo code out of your mind for better understanding rather than > several ping-ping with vague words. diff --git a/mm/compaction.c b/mm/compaction.c index 0409a4ad6ea1..6584705a46f6 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -685,7 +685,8 @@ static bool too_many_isolated(struct zone *zone) */ static unsigned long isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, - unsigned long end_pfn, isolate_mode_t isolate_mode) + unsigned long end_pfn, isolate_mode_t isolate_mode, + unsigned long *isolated_file, unsigned long *isolated_anon) { struct zone *zone = cc->zone; unsigned long nr_scanned = 0, nr_isolated = 0; @@ -866,6 +867,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, /* Successfully isolated */ del_page_from_lru_list(page, lruvec, page_lru(page)); + if (page_is_file_cache(page)) + (*isolated_file)++; + else + (*isolated_anon)++; isolate_success: list_add(&page->lru, &cc->migratepages); Makes more sense? -- Michal Hocko SUSE Labs
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
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.
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.
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.
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); > unlock_page(
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
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 t
Re: [PATCH v2] mm: exclude isolated non-lru pages from NR_ISOLATED_ANON or NR_ISOLATED_FILE.
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(&cc->migratepages)) > return; > > - list_for_each_entry(page, &cc->migratepages, lru) > - count[!!page_is_file_cache(page)]++; > + list_for_each_entry(page, &cc->migratepages, lru) { > + if (likely(!__PageMovable(page))) > + count[!!page_is_file_cache(page)]++; > + } > > mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_ANON, count[0]); > mod_node_page_state(zone->zone_pgdat, NR_ISOLATED_FILE, count[1]); > diff --git a/mm/migrate.c b/mm/migrate.c > index 99250ae..abe48cc 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -168,8 +168,6 @@ void putback_movable_pages(struct list_head *l) > continue; > } > list_del(&page->lru); > - dec_node_page_state(page, NR_ISOLATED_ANON + > - page_is_file_cache(page)); > /* >* We isolated non-lru movable page so here we can use >* __PageMovable because LRU page's mapping cannot have > @@ -185,6 +183,8 @@ void putback_movable_pages(struct list_head *l) > unlock_page(page); > put_page(page); > } else { > + dec_node_page_state(page, NR_ISOLATED_ANON + > + page_is_file_cache(page)); > putback_lru_page(page); > } > } > @@ -1121,8 +1121,9 @@ static ICE_noinline int unmap_and_move(new_page_t > get_new_page, >* restored. >*/ > list_del(&page->lru); > - dec_node_page_state(page, NR_ISOLATED_ANON + > - page_is_file_cache(page)); > + if (likely(!__PageMovable(page))) > + dec_node_page_state(page, NR_ISOLATED_ANON + > + page_is_file_cache(page)); > } > > /* > -- > 1.9.1 -- Michal Hocko SUSE Labs