This was initially sent in responses to Hugh's woes [1] but looks like the
timing was unfortunate for the patches to be picked. So here's a consolidated
resent. Patch 1 is for an older bug that Hugh found when dealing with the mmotm
bugs, Patches 2 and 3 are mmotm fixes.
This doesn't address the problem 1) in Hugh's mail, which Michal Hocko also
hit recently and reminded me to check these patches' status:
> 1. Fix crash in release_pages() from compact_zone() from kcompactd_do_work():
> kcompactd needs to INIT_LIST_HEAD on the new freepages_held list.
This one should be addressed by dropping the following from mmotm from now:
mm-compaction-direct-freepage-allocation-for-async-direct-compaction.patch
As there were objections from Joonsoo and Mel that I would like to try
addressing before posting again.
[1] http://marc.info/?i=alpine.LSU.2.11.1604120005350.1832%40eggly.anvils
Hugh Dickins (2):
mm, cma: prevent nr_isolated_* counters from going negative
mm, compaction: prevent nr_isolated_* from going negative
Vlastimil Babka (1):
mm, compaction: fix crash in get_pfnblock_flags_mask() from
isolate_freepages():
mm/compaction.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
--
2.8.1
This was initially sent in responses to Hugh's woes [1] but looks like the
timing was unfortunate for the patches to be picked. So here's a consolidated
resent. Patch 1 is for an older bug that Hugh found when dealing with the mmotm
bugs, Patches 2 and 3 are mmotm fixes.
This doesn't address the problem 1) in Hugh's mail, which Michal Hocko also
hit recently and reminded me to check these patches' status:
> 1. Fix crash in release_pages() from compact_zone() from kcompactd_do_work():
> kcompactd needs to INIT_LIST_HEAD on the new freepages_held list.
This one should be addressed by dropping the following from mmotm from now:
mm-compaction-direct-freepage-allocation-for-async-direct-compaction.patch
As there were objections from Joonsoo and Mel that I would like to try
addressing before posting again.
[1] http://marc.info/?i=alpine.LSU.2.11.1604120005350.1832%40eggly.anvils
Hugh Dickins (2):
mm, cma: prevent nr_isolated_* counters from going negative
mm, compaction: prevent nr_isolated_* from going negative
Vlastimil Babka (1):
mm, compaction: fix crash in get_pfnblock_flags_mask() from
isolate_freepages():
mm/compaction.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
--
2.8.1
From: Hugh Dickins <[email protected]>
/proc/sys/vm/stat_refresh warns nr_isolated_anon and nr_isolated_file
go increasingly negative under compaction: which would add delay when
should be none, or no delay when should delay. The bug in compaction was
due to a recent mmotm patch, but much older instance of the bug was also
noticed in isolate_migratepages_range() which is used for CMA and
gigantic hugepage allocations.
The bug is caused by putback_movable_pages() in an error path decrementing
the isolated counters without them being previously incremented by
acct_isolated(). Fix isolate_migratepages_range() by removing the error-path
putback, thus reaching acct_isolated() with migratepages still isolated, and
leaving putback to caller like most other places do.
[[email protected]: expanded the changelog]
Fixes: edc2ca612496 ("mm, compaction: move pageblock checks up from isolate_migratepages_range()")
Cc: [email protected]
Cc: Joonsoo Kim <[email protected]>
Signed-off-by: Hugh Dickins <[email protected]>
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/compaction.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 2427fe547a20..759c3ac73ced 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -924,16 +924,8 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
pfn = isolate_migratepages_block(cc, pfn, block_end_pfn,
ISOLATE_UNEVICTABLE);
- /*
- * In case of fatal failure, release everything that might
- * have been isolated in the previous iteration, and signal
- * the failure back to caller.
- */
- if (!pfn) {
- putback_movable_pages(&cc->migratepages);
- cc->nr_migratepages = 0;
+ if (!pfn)
break;
- }
if (cc->nr_migratepages == COMPACT_CLUSTER_MAX)
break;
--
2.8.1
From: Hugh Dickins <[email protected]>
/proc/sys/vm/stat_refresh warns nr_isolated_anon and nr_isolated_file
go increasingly negative under compaction: which would add delay when
should be none, or no delay when should delay. putback_movable_pages()
decrements the NR_ISOLATED counts which acct_isolated() increments,
so isolate_migratepages_block() needs to acct before putback in that
special case. It's also useful to reset cc->nr_migratepages after putback
so we don't needlessly return too early on the COMPACT_CLUSTER_MAX check.
Also it's easier to track the life of cc->migratepages if we don't assign
it to a migratelist variable.
Fixes: mmotm mm-compaction-skip-blocks-where-isolation-fails-in-async-direct-compaction.patch
Signed-off-by: Hugh Dickins <[email protected]>
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/compaction.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 6a49d1b35515..78552009e6ec 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -638,7 +638,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
{
struct zone *zone = cc->zone;
unsigned long nr_scanned = 0, nr_isolated = 0;
- struct list_head *migratelist = &cc->migratepages;
struct lruvec *lruvec;
unsigned long flags = 0;
bool locked = false;
@@ -812,7 +811,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
del_page_from_lru_list(page, lruvec, page_lru(page));
isolate_success:
- list_add(&page->lru, migratelist);
+ list_add(&page->lru, &cc->migratepages);
cc->nr_migratepages++;
nr_isolated++;
@@ -846,9 +845,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
spin_unlock_irqrestore(&zone->lru_lock, flags);
locked = false;
}
- putback_movable_pages(migratelist);
- nr_isolated = 0;
+ acct_isolated(zone, cc);
+ putback_movable_pages(&cc->migratepages);
+ cc->nr_migratepages = 0;
cc->last_migrated_pfn = 0;
+ nr_isolated = 0;
}
if (low_pfn < next_skip_pfn) {
--
2.8.1
In isolate_freepages(), low_pfn was mistakenly initialized to
pageblock_start_pfn() instead of pageblock_end_pfn(), creating a possible
underflow, as described by Hugh:
There's a case when that "block_start_pfn -= pageblock_nr_pages" loop can
pass through 0 and end up trying to access a pageblock before the start of
the mem_map[].
Fixes: mmotm mm-compaction-wrap-calculating-first-and-last-pfn-of-pageblock.patch
Reported-by: Hugh Dickins <[email protected]>
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/compaction.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 759c3ac73ced..6a49d1b35515 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -999,7 +999,7 @@ static void isolate_freepages(struct compact_control *cc)
block_start_pfn = pageblock_start_pfn(cc->free_pfn);
block_end_pfn = min(block_start_pfn + pageblock_nr_pages,
zone_end_pfn(zone));
- low_pfn = pageblock_start_pfn(cc->migrate_pfn);
+ low_pfn = pageblock_end_pfn(cc->migrate_pfn);
/*
* Isolate free pages until enough are available to migrate the
--
2.8.1
On Mon 25-04-16 15:34:26, Vlastimil Babka wrote:
[...]
> > 1. Fix crash in release_pages() from compact_zone() from kcompactd_do_work():
> > kcompactd needs to INIT_LIST_HEAD on the new freepages_held list.
>
> This one should be addressed by dropping the following from mmotm from now:
>
> mm-compaction-direct-freepage-allocation-for-async-direct-compaction.patch
I can confirm that this has healed
[ 481.380167] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 481.382594] IP: [<ffffffff81134dc7>] release_freepages+0x1c/0x7d
I was hitting today during testing something unrelated.
--
Michal Hocko
SUSE Labs
On Mon, Apr 25, 2016 at 03:35:50PM +0200, Vlastimil Babka wrote:
> From: Hugh Dickins <[email protected]>
>
> /proc/sys/vm/stat_refresh warns nr_isolated_anon and nr_isolated_file
> go increasingly negative under compaction: which would add delay when
> should be none, or no delay when should delay. putback_movable_pages()
> decrements the NR_ISOLATED counts which acct_isolated() increments,
> so isolate_migratepages_block() needs to acct before putback in that
> special case. It's also useful to reset cc->nr_migratepages after putback
> so we don't needlessly return too early on the COMPACT_CLUSTER_MAX check.
>
> Also it's easier to track the life of cc->migratepages if we don't assign
> it to a migratelist variable.
>
> Fixes: mmotm mm-compaction-skip-blocks-where-isolation-fails-in-async-direct-compaction.patch
> Signed-off-by: Hugh Dickins <[email protected]>
> Signed-off-by: Vlastimil Babka <[email protected]>
> ---
> mm/compaction.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 6a49d1b35515..78552009e6ec 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -638,7 +638,6 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> {
> struct zone *zone = cc->zone;
> unsigned long nr_scanned = 0, nr_isolated = 0;
> - struct list_head *migratelist = &cc->migratepages;
> struct lruvec *lruvec;
> unsigned long flags = 0;
> bool locked = false;
> @@ -812,7 +811,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> del_page_from_lru_list(page, lruvec, page_lru(page));
>
> isolate_success:
> - list_add(&page->lru, migratelist);
> + list_add(&page->lru, &cc->migratepages);
> cc->nr_migratepages++;
> nr_isolated++;
>
> @@ -846,9 +845,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> spin_unlock_irqrestore(&zone->lru_lock, flags);
> locked = false;
> }
> - putback_movable_pages(migratelist);
> - nr_isolated = 0;
> + acct_isolated(zone, cc);
> + putback_movable_pages(&cc->migratepages);
> + cc->nr_migratepages = 0;
> cc->last_migrated_pfn = 0;
> + nr_isolated = 0;
Is it better to use separate list and merge it cc->migratepages when
finishing instead of using cc->migratepages directly? If
isolate_migratepages() try to isolate more than one page block and keep
isolated page on previous pageblock, this putback all will invalidate
all the previous work. It would be beyond of the scope of this
function. Now, isolate_migratepages() try to isolate the page in one
pageblock so this code is safe. But, I think that removing such
dependency will be helpful in the future. I'm not strongly insisting it
so if you think it's not useful thing, please ignore this comment.
Thanks.
On 04/26/2016 02:55 AM, Joonsoo Kim wrote:
> On Mon, Apr 25, 2016 at 03:35:50PM +0200, Vlastimil Babka wrote:
>> @@ -846,9 +845,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>> spin_unlock_irqrestore(&zone->lru_lock, flags);
>> locked = false;
>> }
>> - putback_movable_pages(migratelist);
>> - nr_isolated = 0;
>> + acct_isolated(zone, cc);
>> + putback_movable_pages(&cc->migratepages);
>> + cc->nr_migratepages = 0;
>> cc->last_migrated_pfn = 0;
>> + nr_isolated = 0;
>
> Is it better to use separate list and merge it cc->migratepages when
> finishing instead of using cc->migratepages directly? If
> isolate_migratepages() try to isolate more than one page block and keep
> isolated page on previous pageblock, this putback all will invalidate
> all the previous work. It would be beyond of the scope of this
> function. Now, isolate_migratepages() try to isolate the page in one
> pageblock so this code is safe. But, I think that removing such
> dependency will be helpful in the future. I'm not strongly insisting it
> so if you think it's not useful thing, please ignore this comment.
migratelist was merely a reference to cc->migratepages, so it wouldn't prevent
the situation you are suggesting. A truly separate list would need to be
appended to cc->migratepages when leaving isolate_migratepages_block() and
there's no need to do that right now.
BTW, can you check patch 1/3? Thanks!
Vlastimil
> Thanks.
>
On Mon, Apr 25, 2016 at 03:35:48PM +0200, Vlastimil Babka wrote:
> From: Hugh Dickins <[email protected]>
>
> /proc/sys/vm/stat_refresh warns nr_isolated_anon and nr_isolated_file
> go increasingly negative under compaction: which would add delay when
> should be none, or no delay when should delay. The bug in compaction was
> due to a recent mmotm patch, but much older instance of the bug was also
> noticed in isolate_migratepages_range() which is used for CMA and
> gigantic hugepage allocations.
>
> The bug is caused by putback_movable_pages() in an error path decrementing
> the isolated counters without them being previously incremented by
> acct_isolated(). Fix isolate_migratepages_range() by removing the error-path
> putback, thus reaching acct_isolated() with migratepages still isolated, and
> leaving putback to caller like most other places do.
>
> [[email protected]: expanded the changelog]
> Fixes: edc2ca612496 ("mm, compaction: move pageblock checks up from isolate_migratepages_range()")
> Cc: [email protected]
> Cc: Joonsoo Kim <[email protected]>
> Signed-off-by: Hugh Dickins <[email protected]>
> Signed-off-by: Vlastimil Babka <[email protected]>
Acked-by: Joonsoo Kim <[email protected]>
Thanks.
On Tue, Apr 26, 2016 at 09:40:45PM +0200, Vlastimil Babka wrote:
> On 04/26/2016 02:55 AM, Joonsoo Kim wrote:
> >On Mon, Apr 25, 2016 at 03:35:50PM +0200, Vlastimil Babka wrote:
> >>@@ -846,9 +845,11 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >> spin_unlock_irqrestore(&zone->lru_lock, flags);
> >> locked = false;
> >> }
> >>- putback_movable_pages(migratelist);
> >>- nr_isolated = 0;
> >>+ acct_isolated(zone, cc);
> >>+ putback_movable_pages(&cc->migratepages);
> >>+ cc->nr_migratepages = 0;
> >> cc->last_migrated_pfn = 0;
> >>+ nr_isolated = 0;
> >
> >Is it better to use separate list and merge it cc->migratepages when
> >finishing instead of using cc->migratepages directly? If
> >isolate_migratepages() try to isolate more than one page block and keep
> >isolated page on previous pageblock, this putback all will invalidate
> >all the previous work. It would be beyond of the scope of this
> >function. Now, isolate_migratepages() try to isolate the page in one
> >pageblock so this code is safe. But, I think that removing such
> >dependency will be helpful in the future. I'm not strongly insisting it
> >so if you think it's not useful thing, please ignore this comment.
>
> migratelist was merely a reference to cc->migratepages, so it
> wouldn't prevent the situation you are suggesting. A truly separate
> list would need to be appended to cc->migratepages when leaving
> isolate_migratepages_block() and there's no need to do that right
> now.
What I suggest is using truly separate list by defining LIST_HEAD(xxx)
on top of the function. But, I'm okay you think that there's no need
to do it right now.
>
> BTW, can you check patch 1/3? Thanks!
Done.
Thanks.