mm, memory_hotplug: redefine memory offline retry logic

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Quoted 4 lines

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

With commit 3aa2823fdf66 ("mm, memory_hotplug: remove timeout from __offline_memory") also applied, it appears to just get stuck forever, and I get lots of:

  [ 1232.112953] INFO: task kworker/3:0:4609 blocked for more than 120 seconds.
  [ 1232.113067]       Not tainted 4.14.0-rc4-gcc6-next-20171009-g49827b9 #1
  [ 1232.113183] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  [ 1232.113319] kworker/3:0     D11984  4609      2 0x00000800
  [ 1232.113416] Workqueue: memcg_kmem_cache memcg_kmem_cache_create_func
  [ 1232.113531] Call Trace:
  [ 1232.113579] [c0000000fb2db7a0] [c0000000fb2db900] 0xc0000000fb2db900 (unreliable)
  [ 1232.113717] [c0000000fb2db970] [c00000000001c964] __switch_to+0x304/0x6e0
  [ 1232.113840] [c0000000fb2dba10] [c000000000a408c0] __schedule+0x2e0/0xa80
  [ 1232.113978] [c0000000fb2dbae0] [c000000000a410a8] schedule+0x48/0xc0
  [ 1232.114113] [c0000000fb2dbb10] [c000000000a44d88] rwsem_down_read_failed+0x128/0x1b0

  [ 1232.114269] [c0000000fb2dbb70] [c0000000001696a8] __percpu_down_read+0x108/0x110
  [ 1232.114426] [c0000000fb2dbba0] [c00000000032e498] get_online_mems+0x68/0x80
  [ 1232.115487] [c0000000fb2dbbc0] [c0000000002c82ec] memcg_create_kmem_cache+0x4c/0x190
  [ 1232.115651] [c0000000fb2dbc60] [c0000000003483b8] memcg_kmem_cache_create_func+0x38/0xf0
  [ 1232.115809] [c0000000fb2dbc90] [c000000000121594] process_one_work+0x2b4/0x590
  [ 1232.115964] [c0000000fb2dbd20] [c000000000121908] worker_thread+0x98/0x5d0
  [ 1232.116095] [c0000000fb2dbdc0] [c00000000012a134] kthread+0x164/0x1b0
  [ 1232.116229] [c0000000fb2dbe30] [c00000000000bae0] ret_from_kernel_thread+0x5c/0x7c

Quoted 36 lines · Michael Ellerman

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Quoted 4 lines

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

OK, that means that start_isolate_page_range should have failed but it hasn't for some reason. I strongly suspect has_unmovable_pages is doing something wrong. Is the kernel text marked somehow? E.g. PageReserved?

In other words, does the diff below helps?

mm/page_alloc.c +3 -0

index 3badcedf96a7..00d042052501 100644
@@ -7368,6 +7368,9 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 
 		page = pfn_to_page(check);
 
+		if (PageReserved(page))
+			return true;
+
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
 		 * We need not scan over tail pages bacause we don't

Quoted 17 lines

With commit 3aa2823fdf66 ("mm, memory_hotplug: remove timeout from __offline_memory") also applied, it appears to just get stuck forever, and I get lots of:

  [ 1232.112953] INFO: task kworker/3:0:4609 blocked for more than 120 seconds.
  [ 1232.113067]       Not tainted 4.14.0-rc4-gcc6-next-20171009-g49827b9 #1
  [ 1232.113183] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  [ 1232.113319] kworker/3:0     D11984  4609      2 0x00000800
  [ 1232.113416] Workqueue: memcg_kmem_cache memcg_kmem_cache_create_func
  [ 1232.113531] Call Trace:
  [ 1232.113579] [c0000000fb2db7a0] [c0000000fb2db900] 0xc0000000fb2db900 (unreliable)
  [ 1232.113717] [c0000000fb2db970] [c00000000001c964] __switch_to+0x304/0x6e0
  [ 1232.113840] [c0000000fb2dba10] [c000000000a408c0] __schedule+0x2e0/0xa80
  [ 1232.113978] [c0000000fb2dbae0] [c000000000a410a8] schedule+0x48/0xc0
  [ 1232.114113] [c0000000fb2dbb10] [c000000000a44d88] rwsem_down_read_failed+0x128/0x1b0

Quoted 8 lines

  [ 1232.114269] [c0000000fb2dbb70] [c0000000001696a8] __percpu_down_read+0x108/0x110
  [ 1232.114426] [c0000000fb2dbba0] [c00000000032e498] get_online_mems+0x68/0x80
  [ 1232.115487] [c0000000fb2dbbc0] [c0000000002c82ec] memcg_create_kmem_cache+0x4c/0x190
  [ 1232.115651] [c0000000fb2dbc60] [c0000000003483b8] memcg_kmem_cache_create_func+0x38/0xf0
  [ 1232.115809] [c0000000fb2dbc90] [c000000000121594] process_one_work+0x2b4/0x590
  [ 1232.115964] [c0000000fb2dbd20] [c000000000121908] worker_thread+0x98/0x5d0
  [ 1232.116095] [c0000000fb2dbdc0] [c00000000012a134] kthread+0x164/0x1b0
  [ 1232.116229] [c0000000fb2dbe30] [c00000000000bae0] ret_from_kernel_thread+0x5c/0x7c

I do not see how this is related to the offline path.

Quoted 34 lines · Michal Hocko

Quoted 33 lines · Michael Ellerman

Quoted 9 lines · Michal Hocko

From: Michal Hocko <[email protected]>
Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Quoted 4 lines

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 6 lines

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

OK, that means that start_isolate_page_range should have failed but it hasn't for some reason. I strongly suspect has_unmovable_pages is doing something wrong. Is the kernel text marked somehow? E.g. PageReserved?

I'm not sure how the text is marked, will have to dig into that.

Quoted 34 lines · Michael Ellerman

Quoted 33 lines · Michal Hocko

Quoted 32 lines · Michael Ellerman

Quoted 9 lines · Michal Hocko

From: Michal Hocko <[email protected]>
Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Quoted 4 lines

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

OK, that means that start_isolate_page_range should have failed but it hasn't for some reason. I strongly suspect has_unmovable_pages is doing something wrong. Is the kernel text marked somehow? E.g. PageReserved?

I'm not sure how the text is marked, will have to dig into that.

Yeah it's reserved:

  $ grep __init_begin /proc/kallsyms
  c000000000d70000 T __init_begin
  $ ./page-types -r -a 0x0,0xd7
               flags	page-count       MB  symbolic-flags			long-symbolic-flags
  0x0000000100000000	       215       13  __________________________r_______________	reserved
               total	       215       13

Quoted 6 lines

  $ grep __init_begin /proc/kallsyms
  c000000000d70000 T __init_begin
  $ ./page-types -r -a 0x0,0xd7
               flags	page-count       MB  symbolic-flags			long-symbolic-flags
  0x0000000100000000	       215       13  __________________________r_______________	reserved
               total	       215       13

Hey Michael,

What tool is this 'page-types' ?

Quoted 37 lines · Michal Hocko

Quoted 36 lines · Michael Ellerman

Quoted 10 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Quoted 4 lines

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 6 lines

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

OK, that means that start_isolate_page_range should have failed but it hasn't for some reason. I strongly suspect has_unmovable_pages is doing something wrong. Is the kernel text marked somehow? E.g. PageReserved?

I'm not sure how the text is marked, will have to dig into that.

Quoted 1 line

In other words, does the diff below helps?

No that doesn't help.

Quoted 20 lines · Michael Ellerman

Quoted 18 lines · Michal Hocko

Quoted 17 lines · Michael Ellerman

Quoted 14 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Isolation has failed here because the page is not on LRU. Most probably because it was on the pcp LRU cache or it has been removed from the LRU already but it hasn't been freed yet. In both cases the page doesn't look non-migrable so retrying more makes sense.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

real 0m0.001s
user 0m0.000s
sys 0m0.001s

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

real 2m0.009s
user 0m0.000s
sys 1m25.035s

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

Quoted 8 lines

Quoted 3 lines

OK, that means that start_isolate_page_range should have failed but it hasn't for some reason. I strongly suspect has_unmovable_pages is doing something wrong. Is the kernel text marked somehow? E.g. PageReserved?

I'm not sure how the text is marked, will have to dig into that.

Quoted 1 line

In other words, does the diff below helps?

Quoted 1 line

No that doesn't help.

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

		return false;

has bailed out early. I would be quite surprised if the kernel text was sitting in the zone movable. The migrate type check is more fishy AFAICS. I can imagine that the kernel text can share the movable or CMA mt block. I am not really familiar with this function but it looks suspicious. So does it help to remove this check?
---

mm/page_alloc.c +0 -3

index 3badcedf96a7..5b4d85ae445c 100644
@@ -7355,9 +7355,6 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	 */
 	if (zone_idx(zone) == ZONE_MOVABLE)
 		return false;
-	mt = get_pageblock_migratetype(page);
-	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))
-		return false;
 
 	pfn = page_to_pfn(page);
 	for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {

On 10/11/2017 08:51 AM, Michal Hocko wrote:

Quoted 6 lines

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

The MIGRATE_MOVABLE check is indeed bogus, because that doesn't guarantee there are no unmovable pages in the block (CMA block OTOH should be a guarantee).

Quoted 1 line · Vlastimil Babka

On 10/11/2017 08:51 AM, Michal Hocko wrote:

[...]

Quoted 7 lines

Quoted 6 lines

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

Quoted 3 lines

The MIGRATE_MOVABLE check is indeed bogus, because that doesn't guarantee there are no unmovable pages in the block (CMA block OTOH should be a guarantee).

OK, thanks for confirmation. I will remove the MIGRATE_MOVABLE check here. Do you think it is worth removing CMA check as well? This is merely an optimization AFAIU because we do not have to check the full pageblockworth of pfns.

Anyway, let's way for Michael to confirm it really helps. If yes I will post a full patch and ask Andrew to add it as a prerequisite for this patch when sending to Linus to prevent the regression.

Quoted 10 lines · Michal Hocko

Quoted 1 line · Vlastimil Babka

On 10/11/2017 08:51 AM, Michal Hocko wrote:

[...]

Quoted 7 lines

Quoted 6 lines

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

Quoted 8 lines

Quoted 3 lines

The MIGRATE_MOVABLE check is indeed bogus, because that doesn't guarantee there are no unmovable pages in the block (CMA block OTOH should be a guarantee).

OK, thanks for confirmation. I will remove the MIGRATE_MOVABLE check here. Do you think it is worth removing CMA check as well? This is merely an optimization AFAIU because we do not have to check the full pageblockworth of pfns.

Actually, we should remove the CMA part as well. It's true that MIGRATE_CMA does guarantee that the *buddy allocator* won't allocate non-MOVABLE pages from the pageblock. But if the memory got allocated as an actual CMA allocation (alloc_contig...) it will almost certainly not be movable.

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Michael Ellerman

Quoted 5 lines · Michal Hocko

Quoted 4 lines · Michael Ellerman

Quoted 3 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

Well, that means offline didn't actually work for that block even before this patch, right? Is it even a movable_node block? I guess not?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

Ah, right. So your complain is really about that the failure is not instant anymore for blocks that can't be offlined.

Quoted 6 lines

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

The MIGRATE_MOVABLE check is indeed bogus, because that doesn't guarantee there are no unmovable pages in the block (CMA block OTOH should be a guarantee).

Quoted 25 lines

mm/page_alloc.c +3 -3

index 3badcedf96a7..5b4d85ae445c 100644
@@ -7355,9 +7355,6 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	 */
 	if (zone_idx(zone) == ZONE_MOVABLE)
 		return false;
-	mt = get_pageblock_migratetype(page);
-	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))
-		return false;
 
 	pfn = page_to_pfn(page);
 	for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {

@@ -7368,6 +7365,9 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 
 		page = pfn_to_page(check);
 
+		if (PageReserved(page))
+			return true;
+
 		/*
 		 * Hugepages are not in LRU lists, but they're movable.
 		 * We need not scan over tail pages bacause we don't

Quoted 8 lines · Vlastimil Babka

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Michael Ellerman

Quoted 5 lines · Michal Hocko

Quoted 4 lines · Michael Ellerman

Quoted 3 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Memory offlining can fail just too eagerly under a heavy memory pressure.

...

Quoted 9 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

This breaks offline for me.

Prior to this commit:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

Well, that means offline didn't actually work for that block even before this patch, right? Is it even a movable_node block? I guess not?

Correct. It should fail.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

After:
/sys/devices/system/memory/memory0# time echo 0 > online -bash: echo: write error: Device or resource busy

real 2m0.009s
user 0m0.000s
sys 1m25.035s

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

There's no way that block can be removed, it contains the kernel text, so it should instantly fail - which it used to.

Ah, right. So your complain is really about that the failure is not instant anymore for blocks that can't be offlined.

Yes. Previously it failed instantly, now it doesn't fail, and loops infinitely (once the 2 minute limit is removed).

Quoted 7 lines

Quoted 6 lines

This is really strange! As you write in other email the page is reserved. That means that some of the earlier checks

	if (zone_idx(zone) == ZONE_MOVABLE)
		return false;
	mt = get_pageblock_migratetype(page);
	if (mt == MIGRATE_MOVABLE || is_migrate_cma(mt))

Quoted 3 lines

The MIGRATE_MOVABLE check is indeed bogus, because that doesn't guarantee there are no unmovable pages in the block (CMA block OTOH should be a guarantee).

OK I'll try that and get back to you.

From: Michal Hocko <[email protected]>

Reserved pages should be completely ignored by the core mm because they have a special meaning for their owners. has_unmovable_pages doesn't check those so we rely on other tests (reference count, or PageLRU) to fail on such pages. Althought this happens to work it is safer to simply check for those explicitly and do not rely on the owner of the page to abuse those fields for special purposes.

Please note that this is more of a further fortification of the code rahter than a fix of an existing issue.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

mm/page_alloc.c +0 -0

index ad0294ab3e4f..a8800b0a5619 100644

Quoted 23 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Reserved pages should be completely ignored by the core mm because they have a special meaning for their owners. has_unmovable_pages doesn't check those so we rely on other tests (reference count, or PageLRU) to fail on such pages. Althought this happens to work it is safer to simply check for those explicitly and do not rely on the owner of the page to abuse those fields for special purposes.

Please note that this is more of a further fortification of the code rahter than a fix of an existing issue.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

mm/page_alloc.c +0 -0

index ad0294ab3e4f..a8800b0a5619 100644
@@ -7365,6 +7365,9 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7368

Quoted 3 lines

mm/page_alloc.c +1 -0

 		page = pfn_to_page(check);
 
+		if (PageReferenced(page))

"Referenced" != "Reserved"

Quoted 24 lines · Vlastimil Babka

Quoted 23 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Reserved pages should be completely ignored by the core mm because they have a special meaning for their owners. has_unmovable_pages doesn't check those so we rely on other tests (reference count, or PageLRU) to fail on such pages. Althought this happens to work it is safer to simply check for those explicitly and do not rely on the owner of the page to abuse those fields for special purposes.

Please note that this is more of a further fortification of the code rahter than a fix of an existing issue.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

mm/page_alloc.c +0 -0

index ad0294ab3e4f..a8800b0a5619 100644
@@ -7365,6 +7365,9 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 

Quoted 5 lines

mm/page_alloc.c bool has_unmovable_pages(struct zone *zone, struct page *page, int count, · line 7368

Quoted 3 lines

mm/page_alloc.c +1 -0

 		page = pfn_to_page(check);
 
+		if (PageReferenced(page))

"Referenced" != "Reserved"

Dohh, you are right of course. I blame auto-completion ;) but I am lame in fact...
---
From 44b20bdb03846bc5fd79c883d16b8f3aa436878f Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Fri, 13 Oct 2017 13:55:21 +0200
Subject: [PATCH] mm, page_alloc: fail has_unmovable_pages when seeing reserved
pages

Reserved pages should be completely ignored by the core mm because they have a special meaning for their owners. has_unmovable_pages doesn't check those so we rely on other tests (reference count, or PageLRU) to fail on such pages. Althought this happens to work it is safer to simply check for those explicitly and do not rely on the owner of the page to abuse those fields for special purposes.

Please note that this is more of a further fortification of the code rahter than a fix of an existing issue.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

mm/page_alloc.c +0 -0

index ad0294ab3e4f..5b4d85ae445c 100644

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Reported-by Michael Ellerman <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Quoted 20 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Quoted 2 lines

Reported-by Michael Ellerman <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Thanks, that works for me.

Tested-by Michael Ellerman <[email protected]>

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Reported-by Michael Ellerman <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/page_alloc.c | 3 ---
 1 file changed, 3 deletions(-)

mm/page_alloc.c +0 -0

index 3badcedf96a7..ad0294ab3e4f 100644

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Quoted 12 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

Quoted 7 lines

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Quoted 21 lines · Joonsoo Kim

Quoted 12 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Michael has noticed that the memory offline tries to migrate kernel code pages when doing
echo 0 > /sys/devices/system/memory/memory0/online

The current implementation will fail the operation after several failed page migration attempts but we shouldn't even attempt to migrate that memory and fail right away because this memory is clearly not migrateable. This will become a real problem when we drop the retry loop counter resp. timeout.

Quoted 7 lines

The real problem is in has_unmovable_pages in fact. We should fail if there are any non migrateable pages in the area. In orther to guarantee that remove the migrate type checks because MIGRATE_MOVABLE is not guaranteed to contain only migrateable pages. It is merely a heuristic. Similarly MIGRATE_CMA does guarantee that the page allocator doesn't allocate any non-migrateable pages from the block but CMA allocations themselves are unlikely to migrateable. Therefore remove both checks.

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

function. Which path is that? Can we do the migrate type test theres?

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Quoted 56 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Quoted 62 lines · Joonsoo Kim

Quoted 60 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Quoted 2 lines

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my understanding is that only CMA allocator really depends on this weird semantic and that does MIGRATE_CMA unconditionally.

Quoted 7 lines

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

How do we guarantee the atomicity?

Quoted 66 lines · Michal Hocko

Quoted 64 lines · Joonsoo Kim

Quoted 56 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

Quoted 3 lines

Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my understanding is that only CMA allocator really depends on this weird semantic and that does MIGRATE_CMA unconditionally.

alloc_contig_range() could be called for partial pages in the pageblock. With your patch, this case also fails unnecessarilly if the other pages in the pageblock is pinned.

Until now, there is no user calling alloc_contig_range() with partial pages except CMA allocator but API could support it.

Quoted 8 lines

Quoted 6 lines

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

How do we guarantee the atomicity?

What atomicity do you mean?

Quoted 71 lines · Joonsoo Kim

Quoted 69 lines · Michal Hocko

Quoted 57 lines · Joonsoo Kim

Quoted 56 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 7 lines

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my understanding is that only CMA allocator really depends on this weird semantic and that does MIGRATE_CMA unconditionally.

Quoted 3 lines

alloc_contig_range() could be called for partial pages in the pageblock. With your patch, this case also fails unnecessarilly if the other pages in the pageblock is pinned.

Is this really the case for GB pages? Do we really want to mess those

with CMA blocks and make those blocks basically unusable because GB pages are rarely (if at all migrateable)?

Quoted 2 lines

Until now, there is no user calling alloc_contig_range() with partial pages except CMA allocator but API could support it.

I disagree. If this is a CMA thing it should stay that way. The semantic is quite confusing already, please let's not make it even worse.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Quoted 3 lines

Quoted 1 line

How do we guarantee the atomicity?

What atomicity do you mean?

Currently we are checking and isolating pages under zone lock. If we split that we are losing atomicity, aren't we.

Quoted 76 lines · Michal Hocko

Quoted 74 lines · Joonsoo Kim

Quoted 63 lines · Michal Hocko

Quoted 62 lines · Joonsoo Kim

Quoted 60 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Quoted 6 lines

Quoted 2 lines

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my understanding is that only CMA allocator really depends on this weird semantic and that does MIGRATE_CMA unconditionally.

alloc_contig_range() could be called for partial pages in the pageblock. With your patch, this case also fails unnecessarilly if the other pages in the pageblock is pinned.

Quoted 1 line

Is this really the case for GB pages? Do we really want to mess those

No, but, as I mentioned already, this API can be called with less pages. I know that there is no user with less pages at this moment but I cannot see any point to reduce this API's capability.

Quoted 8 lines

with CMA blocks and make those blocks basically unusable because GB pages are rarely (if at all migrateable)?

Quoted 2 lines

Until now, there is no user calling alloc_contig_range() with partial pages except CMA allocator but API could support it.

I disagree. If this is a CMA thing it should stay that way. The semantic is quite confusing already, please let's not make it even worse.

It is already used by other component.

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

How do we guarantee the atomicity?

What atomicity do you mean?

Currently we are checking and isolating pages under zone lock. If we split that we are losing atomicity, aren't we.

I think that it can be done easily.

set_migratetype_isolate() {
        lock
        __set_migratetype_isolate();
        unlock
}

set_migratetype_isolate_if_no_unmovable_pages() {
lock
if (has_unmovable_pages())
fail
else
__set_migratetype_isolate()
unlock
}

Quoted 77 lines · Joonsoo Kim

Quoted 76 lines · Michal Hocko

Quoted 74 lines · Joonsoo Kim

Quoted 63 lines · Michal Hocko

Quoted 62 lines · Joonsoo Kim

Quoted 60 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Quoted 6 lines

Quoted 2 lines

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my understanding is that only CMA allocator really depends on this weird semantic and that does MIGRATE_CMA unconditionally.

alloc_contig_range() could be called for partial pages in the pageblock. With your patch, this case also fails unnecessarilly if the other pages in the pageblock is pinned.

Quoted 5 lines

Quoted 1 line

Is this really the case for GB pages? Do we really want to mess those

No, but, as I mentioned already, this API can be called with less pages. I know that there is no user with less pages at this moment but I cannot see any point to reduce this API's capability.

I am still confused. So when exactly would you want to use this api for MIGRATE_MOVABLE and use a partial MIGRATE_CMA pageblock?

Quoted 9 lines

Quoted 8 lines

with CMA blocks and make those blocks basically unusable because GB pages are rarely (if at all migrateable)?

Quoted 2 lines

Until now, there is no user calling alloc_contig_range() with partial pages except CMA allocator but API could support it.

I disagree. If this is a CMA thing it should stay that way. The semantic is quite confusing already, please let's not make it even worse.

Quoted 7 lines

It is already used by other component.

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Quoted 23 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

How do we guarantee the atomicity?

What atomicity do you mean?

Currently we are checking and isolating pages under zone lock. If we split that we are losing atomicity, aren't we.

I think that it can be done easily.

set_migratetype_isolate() {
        lock
        __set_migratetype_isolate();
        unlock
}

Quoted 8 lines

set_migratetype_isolate_if_no_unmovable_pages() {
lock
if (has_unmovable_pages())
fail
else
__set_migratetype_isolate()
unlock
}

So you are essentially suggesting to split the API for alloc_contig_range and hotplug users? Care to send a patch? It is not like I would really love this but I would really like to have this issue addressed because I really do want all other patches which depend on this to be merged in the next release cycle.

That being said, I would much rather see MIGRATE_CMA case special cased than duplicate the already confusing API but I will not insist of course.

On Mon, Oct 23, 2017 at 10:10:09AM +0200, Michal Hocko wrote:

Quoted 7 lines

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Yes, alloc_contig_range() also want to guarantee the similar thing. Major difference between them is 'given pfn range'. memory hotplug works by pageblock unit but alloc_contig_range() doesn't. alloc_contig_range() works by the page unit. However, there is no easy way to isolate individual page so it uses pageblock isolation regardless of 'given pfn range'.

Quoted 3 lines

That being said, I would much rather see MIGRATE_CMA case special cased than duplicate the already confusing API but I will not insist of course.

Okay. I atteach the patch. Andrew, could you revert Michal's series and apply this patch first? Perhaps, Michal will resend his series on top of this one.

Quoted 6 lines

Quoted 5 lines

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

Quoted 7 lines

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Yes, alloc_contig_range() also want to guarantee the similar thing. Major difference between them is 'given pfn range'. memory hotplug works by pageblock unit but alloc_contig_range() doesn't. alloc_contig_range() works by the page unit. However, there is no easy way to isolate individual page so it uses pageblock isolation regardless of 'given pfn range'. In this case, checking movability of all pages on the pageblock would cause the problem as I mentioned before.

Quoted 14 lines · Joonsoo Kim

Quoted 6 lines

Quoted 5 lines

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

Quoted 7 lines

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Quoted 8 lines

Yes, alloc_contig_range() also want to guarantee the similar thing. Major difference between them is 'given pfn range'. memory hotplug works by pageblock unit but alloc_contig_range() doesn't. alloc_contig_range() works by the page unit. However, there is no easy way to isolate individual page so it uses pageblock isolation regardless of 'given pfn range'. In this case, checking movability of all pages on the pageblock would cause the problem as I mentioned before.

I couldn't look too closely yet, but do I understand correctly that the *potential* problem (because as you say there are no such alloc_contig_range callers) you are describing is not newly introduced by Michal's series? Then his patch fixing the introduced regression should be enough for now, and further improvements could be posted on top, and not vice versa? Please don't take it wrong, I agree the current state is a bit of a mess and improvements are welcome. Also it seems to me that Michal is right, and there's nothing preventing alloc_contig_range() to allocate from CMA pageblocks for non-CMA purposes (likely not movable), and that should be also fixed?

Quoted 14 lines · Joonsoo Kim

Quoted 6 lines

Quoted 5 lines

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

Quoted 7 lines

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Quoted 8 lines

Yes, alloc_contig_range() also want to guarantee the similar thing. Major difference between them is 'given pfn range'. memory hotplug works by pageblock unit but alloc_contig_range() doesn't. alloc_contig_range() works by the page unit. However, there is no easy way to isolate individual page so it uses pageblock isolation regardless of 'given pfn range'. In this case, checking movability of all pages on the pageblock would cause the problem as I mentioned before.

I couldn't look too closely yet, but do I understand correctly that the *potential* problem (because as you say there are no such alloc_contig_range callers) you are describing is not newly introduced by Michal's series? Then his patch fixing the introduced regression

should be enough for now, and further improvements could be posted on top, and not vice versa? Please don't take it wrong, I agree the current state is a bit of a mess and improvements are welcome. Also it seems to

me that Michal is right, and there's nothing preventing alloc_contig_range() to allocate from CMA pageblocks for non-CMA purposes (likely not movable), and that should be also fixed?

Quoted 24 lines · Vlastimil Babka

Quoted 14 lines · Joonsoo Kim

Quoted 6 lines

Quoted 5 lines

I'm not sure what is the confusing semantic you mentioned. I think that set_migratetype_isolate() has confusing semantic and should be fixed since making the pageblock isolated doesn't need to check if there is unmovable page or not. Do you think that set_migratetype_isolate() need to check it? If so, why?

Quoted 7 lines

My intuitive understanding of set_migratetype_isolate is that it either suceeds and that means that the given pfn range can be isolated for the given type of allocation (be it movable or cma). No new pages will be allocated from this range to allow converging into a free range in a finit amount of time. At least this is how the hotplug code would like to use it and I suppose that the alloc_contig_range would like to guarantee the same to not rely on a fixed amount of migration attempts.

Quoted 8 lines

Yes, alloc_contig_range() also want to guarantee the similar thing. Major difference between them is 'given pfn range'. memory hotplug works by pageblock unit but alloc_contig_range() doesn't. alloc_contig_range() works by the page unit. However, there is no easy way to isolate individual page so it uses pageblock isolation regardless of 'given pfn range'. In this case, checking movability of all pages on the pageblock would cause the problem as I mentioned before.

Quoted 4 lines

I couldn't look too closely yet, but do I understand correctly that the *potential* problem (because as you say there are no such alloc_contig_range callers) you are describing is not newly introduced by Michal's series? Then his patch fixing the introduced regression

This potential problem exists there before Michal's series if the migratetype of the target pageblock isn't MIGRATE_MOVABLE or MIGRATE_CMA. However, his series enlarges this potential problem surface. It would be the problem now even if the migratetype of the target pageblock is MIGRATE_MOVABLE.

Quoted 3 lines

should be enough for now, and further improvements could be posted on top, and not vice versa? Please don't take it wrong, I agree the current state is a bit of a mess and improvements are welcome. Also it seems to

I'm not very sensitive that which patch is applied first. I can do rebase. But, IMHO, correct applying order is my patch first and then Michal's series.

Anyway, Michal, feel free to do what you think correct.

Quoted 3 lines

me that Michal is right, and there's nothing preventing alloc_contig_range() to allocate from CMA pageblocks for non-CMA purposes (likely not movable), and that should be also fixed?

I noticed the problem you mentioned now and, yes, it should be fixed. My patch will naturally fixes this issue, too.

Quoted 56 lines · Michal Hocko

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Thanks.

.

Quoted 1 line · Joonsoo Kim

On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote:

Quoted 35 lines

Quoted 20 lines

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Quoted 2 lines

Thanks.

Hi Joonsoo,

How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()? Something like the skip_hwpoisoned_pages.

Quoted 3 lines · Xishi Qiu

Quoted 1 line · Joonsoo Kim

On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote:

[...]

Quoted 38 lines

Quoted 37 lines

Quoted 20 lines

Quoted 1 line

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Quoted 4 lines

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Unfortunately, alloc_contig_range() can be called with MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.

I did a more thinking and found that it's strange to check if there is unmovable page in the pageblock during the set_migratetype_isolate(). set_migratetype_isolate() should be just for setting the migratetype of the pageblock. Checking other things should be done by another place, for example, before calling the start_isolate_page_range() in __offline_pages().

Thanks.

Quoted 4 lines

Hi Joonsoo,

How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()? Something like the skip_hwpoisoned_pages.

I believe this is what Joonsoo was proposing actually. I cannot say I

Quoted 26 lines · Michal Hocko

Quoted 2 lines · Joonsoo Kim

Quoted 1 line · Michal Hocko

On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Hello,

This patch will break the CMA user. As you mentioned, CMA allocation itself isn't migrateable. So, after a single page is allocated through CMA allocation, has_unmovable_pages() will return true for this pageblock. Then, futher CMA allocation request to this pageblock will fail because it requires isolating the pageblock.

Hmm, does this mean that the CMA allocation path depends on has_unmovable_pages to return false here even though the memory is not movable? This sounds really strange to me and kind of abuse of this

Quoted 6 lines

Your understanding is correct. Perhaps, abuse or wrong function name.

Quoted 1 line

function. Which path is that? Can we do the migrate type test theres?

alloc_contig_range() -> start_isolate_page_range() -> set_migratetype_isolate() -> has_unmovable_pages()

I see. It seems that the CMA and memory hotplug have a very different view on what should happen during isolation.

Quoted 6 lines

Quoted 3 lines

We can add one argument, 'XXX' to set_migratetype_isolate() and change it to check migrate type rather than has_unmovable_pages() if 'XXX' is specified.

Can we use the migratetype argument and do the special thing for MIGRATE_CMA? Like the following diff?

And with the full changelog.
---
From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Reported-by Joonsoo Kim <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 include/linux/page-isolation.h |  2 +-
 mm/page_alloc.c                | 12 +++++++++++-
 mm/page_isolation.c            | 10 +++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)

include/linux/page-isolation.h +1 -1

index d4cd2014fa6f..fa9db0c7b54e 100644
@@ -30,7 +30,7 @@ static inline bool is_migrate_isolate(int migratetype)
 #endif
 
 bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
-			 bool skip_hwpoisoned_pages);
+			 int migratetype, bool skip_hwpoisoned_pages);
 void set_pageblock_migratetype(struct page *page, int migratetype);
 int move_freepages_block(struct zone *zone, struct page *page,
 				int migratetype, int *num_movable);

mm/page_alloc.c +11 -1

index 5b4d85ae445c..259aeb22462f 100644
@@ -7344,6 +7344,7 @@ void *__init alloc_large_system_hash(const char *tablename,
  * race condition. So you can't expect this function should be exact.
  */
 bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
+			 int migratetype,
 			 bool skip_hwpoisoned_pages)
 {
 	unsigned long pfn, iter, found;

@@ -7356,6 +7357,15 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	if (zone_idx(zone) == ZONE_MOVABLE)
 		return false;
 
+	/*
+	 * CMA allocations (alloc_contig_range) really need to mark isolate
+	 * CMA pageblocks even when they are not movable in fact so consider
+	 * them movable here.
+	 */
+	if (is_migrate_cma(migratetype) &&
+			is_migrate_cma(get_pageblock_migratetype(page)))
+		return false;
+
 	pfn = page_to_pfn(page);
 	for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
 		unsigned long check = pfn + iter;

@@ -7441,7 +7451,7 @@ bool is_pageblock_removable_nolock(struct page *page)
 	if (!zone_spans_pfn(zone, pfn))
 		return false;
 
-	return !has_unmovable_pages(zone, page, 0, true);
+	return !has_unmovable_pages(zone, page, 0, MIGRATE_MOVABLE, true);
 }
 
 #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)

mm/page_isolation.c +4 -4

index 757410d9f758..8616f5332c77 100644
@@ -14,7 +14,7 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/page_isolation.h>
 
-static int set_migratetype_isolate(struct page *page,
+static int set_migratetype_isolate(struct page *page, int migratetype,
 				bool skip_hwpoisoned_pages)
 {
 	struct zone *zone;

@@ -51,7 +51,7 @@ static int set_migratetype_isolate(struct page *page,
 	 * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
 	 * We just check MOVABLE pages.
 	 */
-	if (!has_unmovable_pages(zone, page, arg.pages_found,
+	if (!has_unmovable_pages(zone, page, arg.pages_found, migratetype,
 				 skip_hwpoisoned_pages))
 		ret = 0;
 

@@ -63,14 +63,14 @@ static int set_migratetype_isolate(struct page *page,
 out:
 	if (!ret) {
 		unsigned long nr_pages;
-		int migratetype = get_pageblock_migratetype(page);
+		int mt = get_pageblock_migratetype(page);
 
 		set_pageblock_migratetype(page, MIGRATE_ISOLATE);
 		zone->nr_isolate_pageblock++;
 		nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE,
 									NULL);
 
-		__mod_zone_freepage_state(zone, -nr_pages, migratetype);
+		__mod_zone_freepage_state(zone, -nr_pages, mt);
 	}
 
 	spin_unlock_irqrestore(&zone->lock, flags);

From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 19 Oct 2017 14:14:02 +0200
Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
has_unmovable_pages

Joonsoo has noticed that "mm: drop migrate type checks from has_unmovable_pages" would break CMA allocator because it relies on has_unmovable_pages returning false even for CMA pageblocks which in fact don't have to be movable:
alloc_contig_range
start_isolate_page_range
set_migratetype_isolate
has_unmovable_pages

This is a result of the code sharing between CMA and memory hotplug while each one has a different idea of what has_unmovable_pages should return. This is unfortunate but fixing it properly would require a lot of code duplication.

Fix the issue by introducing the requested migrate type argument and special case MIGRATE_CMA case where CMA page blocks are handled properly. This will work for memory hotplug because it requires MIGRATE_MOVABLE.

Reported-by Joonsoo Kim <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---

 include/linux/page-isolation.h |  2 +-
 mm/page_alloc.c                | 12 +++++++++++-
 mm/page_isolation.c            | 10 +++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)

include/linux/page-isolation.h +1 -1

index d4cd2014fa6f..fa9db0c7b54e 100644
@@ -30,7 +30,7 @@ static inline bool is_migrate_isolate(int migratetype)
 #endif
 
 bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
-			 bool skip_hwpoisoned_pages);
+			 int migratetype, bool skip_hwpoisoned_pages);
 void set_pageblock_migratetype(struct page *page, int migratetype);
 int move_freepages_block(struct zone *zone, struct page *page,
 				int migratetype, int *num_movable);

mm/page_alloc.c +11 -1

index 5b4d85ae445c..259aeb22462f 100644
@@ -7344,6 +7344,7 @@ void *__init alloc_large_system_hash(const char *tablename,
  * race condition. So you can't expect this function should be exact.
  */
 bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
+			 int migratetype,
 			 bool skip_hwpoisoned_pages)
 {
 	unsigned long pfn, iter, found;

@@ -7356,6 +7357,15 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	if (zone_idx(zone) == ZONE_MOVABLE)
 		return false;
 
+	/*
+	 * CMA allocations (alloc_contig_range) really need to mark isolate
+	 * CMA pageblocks even when they are not movable in fact so consider
+	 * them movable here.
+	 */
+	if (is_migrate_cma(migratetype) &&
+			is_migrate_cma(get_pageblock_migratetype(page)))
+		return false;
+
 	pfn = page_to_pfn(page);
 	for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
 		unsigned long check = pfn + iter;

@@ -7441,7 +7451,7 @@ bool is_pageblock_removable_nolock(struct page *page)
 	if (!zone_spans_pfn(zone, pfn))
 		return false;
 
-	return !has_unmovable_pages(zone, page, 0, true);
+	return !has_unmovable_pages(zone, page, 0, MIGRATE_MOVABLE, true);
 }
 
 #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)

mm/page_isolation.c +5 -5

index 757410d9f758..8616f5332c77 100644
@@ -14,7 +14,7 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/page_isolation.h>
 
-static int set_migratetype_isolate(struct page *page,
+static int set_migratetype_isolate(struct page *page, int migratetype,
 				bool skip_hwpoisoned_pages)
 {
 	struct zone *zone;

@@ -51,7 +51,7 @@ static int set_migratetype_isolate(struct page *page,
 	 * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
 	 * We just check MOVABLE pages.
 	 */
-	if (!has_unmovable_pages(zone, page, arg.pages_found,
+	if (!has_unmovable_pages(zone, page, arg.pages_found, migratetype,
 				 skip_hwpoisoned_pages))
 		ret = 0;
 

@@ -63,14 +63,14 @@ static int set_migratetype_isolate(struct page *page,
 out:
 	if (!ret) {
 		unsigned long nr_pages;
-		int migratetype = get_pageblock_migratetype(page);
+		int mt = get_pageblock_migratetype(page);
 
 		set_pageblock_migratetype(page, MIGRATE_ISOLATE);
 		zone->nr_isolate_pageblock++;
 		nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE,
 									NULL);
 
-		__mod_zone_freepage_state(zone, -nr_pages, migratetype);
+		__mod_zone_freepage_state(zone, -nr_pages, mt);
 	}
 
 	spin_unlock_irqrestore(&zone->lock, flags);

@@ -182,7 +182,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
 	     pfn += pageblock_nr_pages) {
 		page = __first_valid_page(pfn, pageblock_nr_pages);
 		if (page &&
-		    set_migratetype_isolate(page, skip_hwpoisoned_pages)) {
+		    set_migratetype_isolate(page, migratetype, skip_hwpoisoned_pages)) {
 			undo_pfn = pfn;
 			goto undo;
 		}

Signature · 5 lines

2.14.2

-- 
Michal Hocko
SUSE Labs