[PATCH 0/3] mainline and mmotm compaction fixes

[PATCH 0/3] mainline and mmotm compaction fixes

[Vlastimil Babka]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 0/3] mainline and mmotm compaction fixes

[Vlastimil Babka]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4.6 1/3] mm, cma: prevent nr_isolated_* counters from going negative

[Vlastimil Babka]

From: Hugh Dickins <hughd@google.com>

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

[vbabka@suse.cz: expanded the changelog]
Fixes: edc2ca612496 ("mm, compaction: move pageblock checks up from isolate_migratepages_range()")
Cc: stable@vger.kernel.org
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH mmotm 2/3] mm, compaction: fix crash in get_pfnblock_flags_mask() from isolate_freepages():

[Vlastimil Babka]

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 <hughd@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from going negative

[Vlastimil Babka]

From: Hugh Dickins <hughd@google.com>

/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 <hughd@google.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/3] mainline and mmotm compaction fixes

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from going negative

[Joonsoo Kim]

On Mon, Apr 25, 2016 at 03:35:50PM +0200, Vlastimil Babka wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> /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 <hughd@google.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from going negative

[Vlastimil Babka]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4.6 1/3] mm, cma: prevent nr_isolated_* counters from going negative

[Joonsoo Kim]

On Mon, Apr 25, 2016 at 03:35:48PM +0200, Vlastimil Babka wrote:
> From: Hugh Dickins <hughd@google.com>
> 
> /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.
> 
> [vbabka@suse.cz: expanded the changelog]
> Fixes: edc2ca612496 ("mm, compaction: move pageblock checks up from isolate_migratepages_range()")
> Cc: stable@vger.kernel.org
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from going negative

[Joonsoo Kim]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>