Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1386443 > unrolled thread

[PATCH 0/3] mainline and mmotm compaction fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] mainline and mmotm compaction fixes Vlastimil Babka <vbabka@suse.cz> - 2016-04-25 15:40 +0200
    [PATCH mmotm 2/3] mm, compaction: fix crash in get_pfnblock_flags_mask() from isolate_freepages(): Vlastimil Babka <vbabka@suse.cz> - 2016-04-25 15:40 +0200
    [PATCH 0/3] mainline and mmotm compaction fixes Vlastimil Babka <vbabka@suse.cz> - 2016-04-25 15:40 +0200
      [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from going negative Vlastimil Babka <vbabka@suse.cz> - 2016-04-25 15:40 +0200
        Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from  going negative Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-04-26 03:00 +0200
          Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from  going negative Vlastimil Babka <vbabka@suse.cz> - 2016-04-26 21:50 +0200
            Re: [PATCH mmotm 3/3] mm, compaction: prevent nr_isolated_* from  going negative Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-04-27 03:00 +0200
    Re: [PATCH 0/3] mainline and mmotm compaction fixes Michal Hocko <mhocko@kernel.org> - 2016-04-25 16:40 +0200

#1386443 — [PATCH 0/3] mainline and mmotm compaction fixes

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1386447

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1386523

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web