This is the same sort of error we saw in [1].
Gigantic hugepages crosses several memblocks, so it can be
that the page we get in scan_movable_pages() is a page-tail
belonging to a 1G-hugepage.
If that happens, page_hstate()->size_to_hstate() will return NULL,
and we will blow up in hugepage_migration_supported().
The splat is as follows:
kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
kernel: #PF error: [normal kernel read fault]
kernel: PGD 0 P4D 0
kernel: Oops: 0000 [#1] SMP PTI
kernel: CPU: 1 PID: 1350 Comm: bash Tainted: G E 5.0.0-rc1-mm1-1-default+ #27
kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
kernel: RIP: 0010:__offline_pages+0x6ae/0x900
kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
kernel: Call Trace:
kernel: ? klist_next+0x79/0xe0
kernel: memory_subsys_offline+0x42/0x60
kernel: device_offline+0x80/0xa0
kernel: state_store+0xab/0xc0
kernel: kernfs_fop_write+0x102/0x180
kernel: __vfs_write+0x26/0x190
kernel: ? set_close_on_exec+0x49/0x70
kernel: vfs_write+0xad/0x1b0
kernel: ksys_write+0x42/0x90
kernel: do_syscall_64+0x5b/0x180
kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
kernel: RIP: 0033:0x7ff1719febe4
kernel: Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 4a fc 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
kernel: RSP: 002b:00007ffd50b7ddc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
kernel: RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007ff1719febe4
kernel: RDX: 0000000000000008 RSI: 00005556e9216b20 RDI: 0000000000000001
kernel: RBP: 00005556e9216b20 R08: 000000000000000a R09: 0000000000000000
kernel: R10: 000000000000000a R11: 0000000000000246 R12: 0000000000000008
kernel: R13: 0000000000000001 R14: 00007ff171cca720 R15: 0000000000000008
kernel: Modules linked in: af_packet(E) xt_tcpudp(E) ipt_REJECT(E) xt_conntrack(E) nf_conntrack(E) nf_defrag_ipv4(E) ip_set(E) nfnetlink(E) ebtable_nat(E) ebtable_broute(E) bridge(E) stp(E) llc(E) iptable_mangle(E) iptable_raw(E) iptable_security(E) ebtable_filter(E) ebtables(E) iptable_filter(E) ip_tables(E) x_tables(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) bochs_drm(E) ttm(E) aesni_intel(E) drm_kms_helper(E) aes_x86_64(E) crypto_simd(E) cryptd(E) glue_helper(E) drm(E) virtio_net(E) syscopyarea(E) sysfillrect(E) net_failover(E) sysimgblt(E) pcspkr(E) failover(E) i2c_piix4(E) fb_sys_fops(E) parport_pc(E) parport(E) button(E) btrfs(E) libcrc32c(E) xor(E) zstd_decompress(E) zstd_compress(E) xxhash(E) raid6_pq(E) sd_mod(E) ata_generic(E) ata_piix(E) ahci(E) libahci(E) libata(E) crc32c_intel(E) serio_raw(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E) scsi_mod(E) autofs4(E)
kernel: CR2: 0000000000000008
kernel: ---[ end trace bdb71590872849fb ]---
kernel: RIP: 0010:__offline_pages+0x6ae/0x900
kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Fix this by getting the head page and testing against it.
[1] https://patchwork.kernel.org/patch/10739963/
Signed-off-by: Oscar Salvador <[email protected]>
---
mm/memory_hotplug.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index ec22c86d9f89..25aee4f04a72 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1335,12 +1335,17 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
if (__PageMovable(page))
return pfn;
if (PageHuge(page)) {
- if (hugepage_migration_supported(page_hstate(page)) &&
- page_huge_active(page))
+ struct page *head = compound_head(page);
+
+ if (hugepage_migration_supported(page_hstate(head)) &&
+ page_huge_active(head))
return pfn;
- else
- pfn = round_up(pfn + 1,
- 1 << compound_order(page)) - 1;
+ else {
+ unsigned long skip;
+
+ skip = (1 << compound_order(head)) - (page - head);
+ pfn += skip - 1;
+ }
}
}
}
--
2.13.7
On 1/22/19 7:44 AM, Oscar Salvador wrote:
> This is the same sort of error we saw in [1].
>
> Gigantic hugepages crosses several memblocks, so it can be
> that the page we get in scan_movable_pages() is a page-tail
> belonging to a 1G-hugepage.
> If that happens, page_hstate()->size_to_hstate() will return NULL,
> and we will blow up in hugepage_migration_supported().
>
> The splat is as follows:
>
> kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> kernel: #PF error: [normal kernel read fault]
> kernel: PGD 0 P4D 0
> kernel: Oops: 0000 [#1] SMP PTI
> kernel: CPU: 1 PID: 1350 Comm: bash Tainted: G E 5.0.0-rc1-mm1-1-default+ #27
> kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> kernel: Call Trace:
> kernel: ? klist_next+0x79/0xe0
> kernel: memory_subsys_offline+0x42/0x60
> kernel: device_offline+0x80/0xa0
> kernel: state_store+0xab/0xc0
> kernel: kernfs_fop_write+0x102/0x180
> kernel: __vfs_write+0x26/0x190
> kernel: ? set_close_on_exec+0x49/0x70
> kernel: vfs_write+0xad/0x1b0
> kernel: ksys_write+0x42/0x90
> kernel: do_syscall_64+0x5b/0x180
> kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
> kernel: RIP: 0033:0x7ff1719febe4
> kernel: Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 4a fc 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> kernel: RSP: 002b:00007ffd50b7ddc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> kernel: RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007ff1719febe4
> kernel: RDX: 0000000000000008 RSI: 00005556e9216b20 RDI: 0000000000000001
> kernel: RBP: 00005556e9216b20 R08: 000000000000000a R09: 0000000000000000
> kernel: R10: 000000000000000a R11: 0000000000000246 R12: 0000000000000008
> kernel: R13: 0000000000000001 R14: 00007ff171cca720 R15: 0000000000000008
> kernel: Modules linked in: af_packet(E) xt_tcpudp(E) ipt_REJECT(E) xt_conntrack(E) nf_conntrack(E) nf_defrag_ipv4(E) ip_set(E) nfnetlink(E) ebtable_nat(E) ebtable_broute(E) bridge(E) stp(E) llc(E) iptable_mangle(E) iptable_raw(E) iptable_security(E) ebtable_filter(E) ebtables(E) iptable_filter(E) ip_tables(E) x_tables(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) bochs_drm(E) ttm(E) aesni_intel(E) drm_kms_helper(E) aes_x86_64(E) crypto_simd(E) cryptd(E) glue_helper(E) drm(E) virtio_net(E) syscopyarea(E) sysfillrect(E) net_failover(E) sysimgblt(E) pcspkr(E) failover(E) i2c_piix4(E) fb_sys_fops(E) parport_pc(E) parport(E) button(E) btrfs(E) libcrc32c(E) xor(E) zstd_decompress(E) zstd_compress(E) xxhash(E) raid6_pq(E) sd_mod(E) ata_generic(E) ata_piix(E) ahci(E) libahci(E) libata(E) crc32c_intel(E) serio_raw(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E) scsi_mod(E) autofs4(E)
> kernel: CR2: 0000000000000008
> kernel: ---[ end trace bdb71590872849fb ]---
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> Fix this by getting the head page and testing against it.
>
> [1] https://patchwork.kernel.org/patch/10739963/
>
> Signed-off-by: Oscar Salvador <[email protected]>
Looks good.
Reviewed-by: Anthony Yznaga <[email protected]>
> ---
> mm/memory_hotplug.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ec22c86d9f89..25aee4f04a72 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1335,12 +1335,17 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> if (__PageMovable(page))
> return pfn;
> if (PageHuge(page)) {
> - if (hugepage_migration_supported(page_hstate(page)) &&
> - page_huge_active(page))
> + struct page *head = compound_head(page);
> +
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> return pfn;
> - else
> - pfn = round_up(pfn + 1,
> - 1 << compound_order(page)) - 1;
> + else {
> + unsigned long skip;
> +
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
> + }
> }
> }
> }
On Tue 22-01-19 16:44:07, Oscar Salvador wrote:
> This is the same sort of error we saw in [1].
>
> Gigantic hugepages crosses several memblocks, so it can be
> that the page we get in scan_movable_pages() is a page-tail
> belonging to a 1G-hugepage.
> If that happens, page_hstate()->size_to_hstate() will return NULL,
> and we will blow up in hugepage_migration_supported().
>
> The splat is as follows:
>
> kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> kernel: #PF error: [normal kernel read fault]
> kernel: PGD 0 P4D 0
> kernel: Oops: 0000 [#1] SMP PTI
> kernel: CPU: 1 PID: 1350 Comm: bash Tainted: G E 5.0.0-rc1-mm1-1-default+ #27
> kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> kernel: Call Trace:
> kernel: ? klist_next+0x79/0xe0
> kernel: memory_subsys_offline+0x42/0x60
> kernel: device_offline+0x80/0xa0
> kernel: state_store+0xab/0xc0
> kernel: kernfs_fop_write+0x102/0x180
> kernel: __vfs_write+0x26/0x190
> kernel: ? set_close_on_exec+0x49/0x70
> kernel: vfs_write+0xad/0x1b0
> kernel: ksys_write+0x42/0x90
> kernel: do_syscall_64+0x5b/0x180
> kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
> kernel: RIP: 0033:0x7ff1719febe4
> kernel: Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 4a fc 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> kernel: RSP: 002b:00007ffd50b7ddc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> kernel: RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007ff1719febe4
> kernel: RDX: 0000000000000008 RSI: 00005556e9216b20 RDI: 0000000000000001
> kernel: RBP: 00005556e9216b20 R08: 000000000000000a R09: 0000000000000000
> kernel: R10: 000000000000000a R11: 0000000000000246 R12: 0000000000000008
> kernel: R13: 0000000000000001 R14: 00007ff171cca720 R15: 0000000000000008
> kernel: Modules linked in: af_packet(E) xt_tcpudp(E) ipt_REJECT(E) xt_conntrack(E) nf_conntrack(E) nf_defrag_ipv4(E) ip_set(E) nfnetlink(E) ebtable_nat(E) ebtable_broute(E) bridge(E) stp(E) llc(E) iptable_mangle(E) iptable_raw(E) iptable_security(E) ebtable_filter(E) ebtables(E) iptable_filter(E) ip_tables(E) x_tables(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) bochs_drm(E) ttm(E) aesni_intel(E) drm_kms_helper(E) aes_x86_64(E) crypto_simd(E) cryptd(E) glue_helper(E) drm(E) virtio_net(E) syscopyarea(E) sysfillrect(E) net_failover(E) sysimgblt(E) pcspkr(E) failover(E) i2c_piix4(E) fb_sys_fops(E) parport_pc(E) parport(E) button(E) btrfs(E) libcrc32c(E) xor(E) zstd_decompress(E) zstd_compress(E) xxhash(E) raid6_pq(E) sd_mod(E) ata_generic(E) ata_piix(E) ahci(E) libahci(E) libata(E) crc32c_intel(E) serio_raw(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E) scsi_mod(E) autofs4(E)
> kernel: CR2: 0000000000000008
> kernel: ---[ end trace bdb71590872849fb ]---
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> Fix this by getting the head page and testing against it.
>
> [1] https://patchwork.kernel.org/patch/10739963/
So this should be probably folded into the above patch as it is
incomplete unless I am missing something.
> Signed-off-by: Oscar Salvador <[email protected]>
Other than that the change looks good to me.
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/memory_hotplug.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ec22c86d9f89..25aee4f04a72 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1335,12 +1335,17 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> if (__PageMovable(page))
> return pfn;
> if (PageHuge(page)) {
> - if (hugepage_migration_supported(page_hstate(page)) &&
> - page_huge_active(page))
> + struct page *head = compound_head(page);
> +
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> return pfn;
> - else
> - pfn = round_up(pfn + 1,
> - 1 << compound_order(page)) - 1;
> + else {
> + unsigned long skip;
> +
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
> + }
> }
> }
> }
> --
> 2.13.7
--
Michal Hocko
SUSE Labs
On Wed, Jan 23, 2019 at 10:47:17AM +0100, Michal Hocko wrote:
> So this should be probably folded into the above patch as it is
> incomplete unless I am missing something.
Well, they are triggered from different paths.
The former error was triggered in:
removable_show
is_mem_section_removable
is_pageblock_removable_nolock
has_unmovable_pages
while this one is triggered when actually doing the offline operation
__offline_pages
scan_movable_pages
But I do agree that one without the other is not really useful, an incomplete.
The truth is that I did not spot this one when fixing [1] because I did not
really try to offline the memblock back then, so my fault.
While I agree that the best approach would be to fold this one into [1],
I am not sure if it is too late for that as it seems that [1] was already
released into mainline, and moreover to stable.
I guess I will have Andrew decide what is the best way to carry on here.
[1] https://patchwork.kernel.org/patch/10739963/
>
> > Signed-off-by: Oscar Salvador <[email protected]>
>
> Other than that the change looks good to me.
>
> Acked-by: Michal Hocko <[email protected]>
Thanks!
--
Oscar Salvador
SUSE L3
On 22.01.19 16:44, Oscar Salvador wrote:
> This is the same sort of error we saw in [1].
>
> Gigantic hugepages crosses several memblocks, so it can be
> that the page we get in scan_movable_pages() is a page-tail
> belonging to a 1G-hugepage.
> If that happens, page_hstate()->size_to_hstate() will return NULL,
> and we will blow up in hugepage_migration_supported().
>
> The splat is as follows:
>
> kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> kernel: #PF error: [normal kernel read fault]
> kernel: PGD 0 P4D 0
> kernel: Oops: 0000 [#1] SMP PTI
> kernel: CPU: 1 PID: 1350 Comm: bash Tainted: G E 5.0.0-rc1-mm1-1-default+ #27
> kernel: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> kernel: Call Trace:
> kernel: ? klist_next+0x79/0xe0
> kernel: memory_subsys_offline+0x42/0x60
> kernel: device_offline+0x80/0xa0
> kernel: state_store+0xab/0xc0
> kernel: kernfs_fop_write+0x102/0x180
> kernel: __vfs_write+0x26/0x190
> kernel: ? set_close_on_exec+0x49/0x70
> kernel: vfs_write+0xad/0x1b0
> kernel: ksys_write+0x42/0x90
> kernel: do_syscall_64+0x5b/0x180
> kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9
> kernel: RIP: 0033:0x7ff1719febe4
> kernel: Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 4a fc 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
> kernel: RSP: 002b:00007ffd50b7ddc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> kernel: RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007ff1719febe4
> kernel: RDX: 0000000000000008 RSI: 00005556e9216b20 RDI: 0000000000000001
> kernel: RBP: 00005556e9216b20 R08: 000000000000000a R09: 0000000000000000
> kernel: R10: 000000000000000a R11: 0000000000000246 R12: 0000000000000008
> kernel: R13: 0000000000000001 R14: 00007ff171cca720 R15: 0000000000000008
> kernel: Modules linked in: af_packet(E) xt_tcpudp(E) ipt_REJECT(E) xt_conntrack(E) nf_conntrack(E) nf_defrag_ipv4(E) ip_set(E) nfnetlink(E) ebtable_nat(E) ebtable_broute(E) bridge(E) stp(E) llc(E) iptable_mangle(E) iptable_raw(E) iptable_security(E) ebtable_filter(E) ebtables(E) iptable_filter(E) ip_tables(E) x_tables(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) bochs_drm(E) ttm(E) aesni_intel(E) drm_kms_helper(E) aes_x86_64(E) crypto_simd(E) cryptd(E) glue_helper(E) drm(E) virtio_net(E) syscopyarea(E) sysfillrect(E) net_failover(E) sysimgblt(E) pcspkr(E) failover(E) i2c_piix4(E) fb_sys_fops(E) parport_pc(E) parport(E) button(E) btrfs(E) libcrc32c(E) xor(E) zstd_decompress(E) zstd_compress(E) xxhash(E) raid6_pq(E) sd_mod(E) ata_generic(E) ata_piix(E) ahci(E) libahci(E) libata(E) crc32c_intel(E) serio_raw(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E) scsi_mod(E) autofs4(E)
> kernel: CR2: 0000000000000008
> kernel: ---[ end trace bdb71590872849fb ]---
> kernel: RIP: 0010:__offline_pages+0x6ae/0x900
> kernel: Code: 48 c7 c6 d0 3e a4 81 e8 44 c8 ad ff 49 8b 04 24 bf 00 10 00 00 a9 00 00 01 00 74 09 41 0f b6 4c 24 51 48 d3 e7 e8 42 2a c1 ff <8b> 40 08 83 f8 09 0f 84 b0 fc ff ff 83 f8 12 0f 84 a7 fc ff ff 83
> kernel: RSP: 0018:ffffc900008e3d20 EFLAGS: 00010246
> kernel: RAX: 0000000000000000 RBX: ffffea0000000000 RCX: 0000000000000009
> kernel: RDX: ffffffff825c64f0 RSI: 0000000000001000 RDI: 0000000000001000
> kernel: RBP: ffffc900008e3d68 R08: 0000000000200000 R09: 00000000000001e4
> kernel: R10: 0000000000000058 R11: ffffffff8254a854 R12: ffffea0004200000
> kernel: R13: 0000000000108000 R14: 0000000000110000 R15: 0000000000000000
> kernel: FS: 00007ff172339b80(0000) GS:ffff88803eb00000(0000) knlGS:0000000000000000
> kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: CR2: 0000000000000008 CR3: 0000000038d78006 CR4: 00000000003606a0
> kernel: DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> kernel: DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> Fix this by getting the head page and testing against it.
>
> [1] https://patchwork.kernel.org/patch/10739963/
>
> Signed-off-by: Oscar Salvador <[email protected]>
> ---
> mm/memory_hotplug.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ec22c86d9f89..25aee4f04a72 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1335,12 +1335,17 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> if (__PageMovable(page))
> return pfn;
> if (PageHuge(page)) {
> - if (hugepage_migration_supported(page_hstate(page)) &&
> - page_huge_active(page))
> + struct page *head = compound_head(page);
> +
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> return pfn;
> - else
> - pfn = round_up(pfn + 1,
> - 1 << compound_order(page)) - 1;
> + else {
If you use {} for the else case, please also do so for the if case.
Apart from that this looks good to me
Reviewed-by: David Hildenbrand <[email protected]>
> + unsigned long skip;
> +
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
> + }
> }
> }
> }
>
--
Thanks,
David / dhildenb
On Wed 23-01-19 11:18:42, Oscar Salvador wrote:
> On Wed, Jan 23, 2019 at 10:47:17AM +0100, Michal Hocko wrote:
> > So this should be probably folded into the above patch as it is
> > incomplete unless I am missing something.
>
> Well, they are triggered from different paths.
> The former error was triggered in:
>
> removable_show
> is_mem_section_removable
> is_pageblock_removable_nolock
> has_unmovable_pages
>
> while this one is triggered when actually doing the offline operation
But it would trigger from the offline path as well, no?
> __offline_pages
> scan_movable_pages
>
> But I do agree that one without the other is not really useful, an incomplete.
> The truth is that I did not spot this one when fixing [1] because I did not
> really try to offline the memblock back then, so my fault.
I should have noticed that during the review but those paths are really
far away from each other so this is hard to spot indeed
> While I agree that the best approach would be to fold this one into [1],
> I am not sure if it is too late for that as it seems that [1] was already
> released into mainline, and moreover to stable.
OK, I wasn't aware of that. Then my suggestion is clearly moot.
> I guess I will have Andrew decide what is the best way to carry on here.
>
> [1] https://patchwork.kernel.org/patch/10739963/
>
> >
> > > Signed-off-by: Oscar Salvador <[email protected]>
> >
> > Other than that the change looks good to me.
> >
> > Acked-by: Michal Hocko <[email protected]>
>
> Thanks!
> --
> Oscar Salvador
> SUSE L3
--
Michal Hocko
SUSE Labs
On Wed, Jan 23, 2019 at 11:33:56AM +0100, David Hildenbrand wrote:
> If you use {} for the else case, please also do so for the if case.
Diff on top:
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 25aee4f04a72..d5810e522b72 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1338,9 +1338,9 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
struct page *head = compound_head(page);
if (hugepage_migration_supported(page_hstate(head)) &&
- page_huge_active(head))
+ page_huge_active(head)) {
return pfn;
- else {
+ } else {
unsigned long skip;
skip = (1 << compound_order(head)) - (page - head);
> Apart from that this looks good to me
>
> Reviewed-by: David Hildenbrand <[email protected]>
Thanks David ;-)
--
Oscar Salvador
SUSE L3
On Tue, 22 Jan 2019 16:44:07 +0100 Oscar Salvador <[email protected]> wrote:
> This is the same sort of error we saw in [1].
I'll replace "[1]" with 17e2e7d7e1b83 ("mm, page_alloc: fix
has_unmovable_pages for HugePages").
> Signed-off-by: Oscar Salvador <[email protected]>
And I'll add cc:stable.
On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <[email protected]> wrote:
> On Wed, Jan 23, 2019 at 11:33:56AM +0100, David Hildenbrand wrote:
> > If you use {} for the else case, please also do so for the if case.
>
> Diff on top:
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 25aee4f04a72..d5810e522b72 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1338,9 +1338,9 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> struct page *head = compound_head(page);
>
> if (hugepage_migration_supported(page_hstate(head)) &&
> - page_huge_active(head))
> + page_huge_active(head)) {
> return pfn;
> - else {
> + } else {
> unsigned long skip;
>
> skip = (1 << compound_order(head)) - (page - head);
>
The indenting is getting a bit deep also, so how about this?
static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
{
unsigned long pfn;
for (pfn = start; pfn < end; pfn++) {
struct page *page, *head;
if (!pfn_valid(pfn))
continue;
page = pfn_to_page(pfn);
if (PageLRU(page))
return pfn;
if (__PageMovable(page))
return pfn;
if (!PageHuge(page))
continue;
head = compound_head(page);
if (hugepage_migration_supported(page_hstate(head)) &&
page_huge_active(head)) {
return pfn;
} else {
unsigned long skip;
skip = (1 << compound_order(head)) - (page - head);
pfn += skip - 1;
}
}
return 0;
}
From: Andrew Morton <[email protected]>
Subject: mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
fix brace layout, per David. Also reduce indentation
Cc: Anthony Yznaga <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Oscar Salvador <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
mm/memory_hotplug.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
--- a/mm/memory_hotplug.c~mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
+++ a/mm/memory_hotplug.c
@@ -1305,27 +1305,29 @@ int test_pages_in_a_zone(unsigned long s
static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
{
unsigned long pfn;
- struct page *page;
+
for (pfn = start; pfn < end; pfn++) {
- if (pfn_valid(pfn)) {
- page = pfn_to_page(pfn);
- if (PageLRU(page))
- return pfn;
- if (__PageMovable(page))
- return pfn;
- if (PageHuge(page)) {
- struct page *head = compound_head(page);
+ struct page *page, *head;
+
+ if (!pfn_valid(pfn))
+ continue;
+ page = pfn_to_page(pfn);
+ if (PageLRU(page))
+ return pfn;
+ if (__PageMovable(page))
+ return pfn;
- if (hugepage_migration_supported(page_hstate(head)) &&
- page_huge_active(head))
- return pfn;
- else {
- unsigned long skip;
+ if (!PageHuge(page))
+ continue;
+ head = compound_head(page);
+ if (hugepage_migration_supported(page_hstate(head)) &&
+ page_huge_active(head)) {
+ return pfn;
+ } else {
+ unsigned long skip;
- skip = (1 << compound_order(head)) - (page - head);
- pfn += skip - 1;
- }
- }
+ skip = (1 << compound_order(head)) - (page - head);
+ pfn += skip - 1;
}
}
return 0;
_
On Mon, 28 Jan 2019 14:53:09 -0800 Andrew Morton <[email protected]> wrote:
> On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <[email protected]> wrote:
>
> > On Wed, Jan 23, 2019 at 11:33:56AM +0100, David Hildenbrand wrote:
> > > If you use {} for the else case, please also do so for the if case.
> >
> > Diff on top:
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 25aee4f04a72..d5810e522b72 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1338,9 +1338,9 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> > struct page *head = compound_head(page);
> >
> > if (hugepage_migration_supported(page_hstate(head)) &&
> > - page_huge_active(head))
> > + page_huge_active(head)) {
> > return pfn;
> > - else {
> > + } else {
> > unsigned long skip;
> >
> > skip = (1 << compound_order(head)) - (page - head);
> >
>
> The indenting is getting a bit deep also, so how about this?
>
> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> {
> unsigned long pfn;
>
> for (pfn = start; pfn < end; pfn++) {
> struct page *page, *head;
>
> if (!pfn_valid(pfn))
> continue;
> page = pfn_to_page(pfn);
> if (PageLRU(page))
> return pfn;
> if (__PageMovable(page))
> return pfn;
>
> if (!PageHuge(page))
> continue;
> head = compound_head(page);
> if (hugepage_migration_supported(page_hstate(head)) &&
> page_huge_active(head)) {
> return pfn;
checkpatch pointed out that else-after-return isn't needed so we can do
static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
{
unsigned long pfn;
for (pfn = start; pfn < end; pfn++) {
struct page *page, *head;
unsigned long skip;
if (!pfn_valid(pfn))
continue;
page = pfn_to_page(pfn);
if (PageLRU(page))
return pfn;
if (__PageMovable(page))
return pfn;
if (!PageHuge(page))
continue;
head = compound_head(page);
if (hugepage_migration_supported(page_hstate(head)) &&
page_huge_active(head))
return pfn;
skip = (1 << compound_order(head)) - (page - head);
pfn += skip - 1;
}
return 0;
}
--- a/mm/memory_hotplug.c~mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
+++ a/mm/memory_hotplug.c
@@ -1305,28 +1305,27 @@ int test_pages_in_a_zone(unsigned long s
static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
{
unsigned long pfn;
- struct page *page;
+
for (pfn = start; pfn < end; pfn++) {
- if (pfn_valid(pfn)) {
- page = pfn_to_page(pfn);
- if (PageLRU(page))
- return pfn;
- if (__PageMovable(page))
- return pfn;
- if (PageHuge(page)) {
- struct page *head = compound_head(page);
+ struct page *page, *head;
+ unsigned long skip;
- if (hugepage_migration_supported(page_hstate(head)) &&
- page_huge_active(head))
- return pfn;
- else {
- unsigned long skip;
+ if (!pfn_valid(pfn))
+ continue;
+ page = pfn_to_page(pfn);
+ if (PageLRU(page))
+ return pfn;
+ if (__PageMovable(page))
+ return pfn;
- skip = (1 << compound_order(head)) - (page - head);
- pfn += skip - 1;
- }
- }
- }
+ if (!PageHuge(page))
+ continue;
+ head = compound_head(page);
+ if (hugepage_migration_supported(page_hstate(head)) &&
+ page_huge_active(head))
+ return pfn;
+ skip = (1 << compound_order(head)) - (page - head);
+ pfn += skip - 1;
}
return 0;
}
_
On Mon 28-01-19 14:56:17, Andrew Morton wrote:
[...]
> --- a/mm/memory_hotplug.c~mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
> +++ a/mm/memory_hotplug.c
> @@ -1305,28 +1305,27 @@ int test_pages_in_a_zone(unsigned long s
> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> {
> unsigned long pfn;
> - struct page *page;
> +
> for (pfn = start; pfn < end; pfn++) {
> - if (pfn_valid(pfn)) {
> - page = pfn_to_page(pfn);
> - if (PageLRU(page))
> - return pfn;
> - if (__PageMovable(page))
> - return pfn;
> - if (PageHuge(page)) {
> - struct page *head = compound_head(page);
> + struct page *page, *head;
> + unsigned long skip;
>
> - if (hugepage_migration_supported(page_hstate(head)) &&
> - page_huge_active(head))
> - return pfn;
> - else {
> - unsigned long skip;
> + if (!pfn_valid(pfn))
> + continue;
> + page = pfn_to_page(pfn);
> + if (PageLRU(page))
> + return pfn;
> + if (__PageMovable(page))
> + return pfn;
>
> - skip = (1 << compound_order(head)) - (page - head);
> - pfn += skip - 1;
> - }
> - }
> - }
> + if (!PageHuge(page))
> + continue;
> + head = compound_head(page);
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> + return pfn;
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
> }
> return 0;
> }
> _
>
LGTM
--
Michal Hocko
SUSE Labs
On Mon, Jan 28, 2019 at 02:56:17PM -0800, Andrew Morton wrote:
>
> --- a/mm/memory_hotplug.c~mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
> +++ a/mm/memory_hotplug.c
> @@ -1305,28 +1305,27 @@ int test_pages_in_a_zone(unsigned long s
> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> {
> unsigned long pfn;
> - struct page *page;
> +
> for (pfn = start; pfn < end; pfn++) {
> - if (pfn_valid(pfn)) {
> - page = pfn_to_page(pfn);
> - if (PageLRU(page))
> - return pfn;
> - if (__PageMovable(page))
> - return pfn;
> - if (PageHuge(page)) {
> - struct page *head = compound_head(page);
> + struct page *page, *head;
> + unsigned long skip;
>
> - if (hugepage_migration_supported(page_hstate(head)) &&
> - page_huge_active(head))
> - return pfn;
> - else {
> - unsigned long skip;
> + if (!pfn_valid(pfn))
> + continue;
> + page = pfn_to_page(pfn);
> + if (PageLRU(page))
> + return pfn;
> + if (__PageMovable(page))
> + return pfn;
>
> - skip = (1 << compound_order(head)) - (page - head);
> - pfn += skip - 1;
> - }
> - }
> - }
> + if (!PageHuge(page))
> + continue;
> + head = compound_head(page);
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> + return pfn;
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
> }
> return 0;
> }
It looks much better, thanks a lot for the cleanup Andrew!
--
Oscar Salvador
SUSE L3
On 28.01.19 23:56, Andrew Morton wrote:
> On Mon, 28 Jan 2019 14:53:09 -0800 Andrew Morton <[email protected]> wrote:
>
>> On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <[email protected]> wrote:
>>
>>> On Wed, Jan 23, 2019 at 11:33:56AM +0100, David Hildenbrand wrote:
>>>> If you use {} for the else case, please also do so for the if case.
>>>
>>> Diff on top:
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 25aee4f04a72..d5810e522b72 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1338,9 +1338,9 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
>>> struct page *head = compound_head(page);
>>>
>>> if (hugepage_migration_supported(page_hstate(head)) &&
>>> - page_huge_active(head))
>>> + page_huge_active(head)) {
>>> return pfn;
>>> - else {
>>> + } else {
>>> unsigned long skip;
>>>
>>> skip = (1 << compound_order(head)) - (page - head);
>>>
>>
>> The indenting is getting a bit deep also, so how about this?
>>
>> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
>> {
>> unsigned long pfn;
>>
>> for (pfn = start; pfn < end; pfn++) {
>> struct page *page, *head;
>>
>> if (!pfn_valid(pfn))
>> continue;
>> page = pfn_to_page(pfn);
>> if (PageLRU(page))
>> return pfn;
>> if (__PageMovable(page))
>> return pfn;
>>
>> if (!PageHuge(page))
>> continue;
>> head = compound_head(page);
>> if (hugepage_migration_supported(page_hstate(head)) &&
>> page_huge_active(head)) {
>> return pfn;
>
> checkpatch pointed out that else-after-return isn't needed so we can do
>
> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> {
> unsigned long pfn;
>
> for (pfn = start; pfn < end; pfn++) {
> struct page *page, *head;
> unsigned long skip;
>
> if (!pfn_valid(pfn))
> continue;
> page = pfn_to_page(pfn);
> if (PageLRU(page))
> return pfn;
> if (__PageMovable(page))
> return pfn;
>
> if (!PageHuge(page))
> continue;
> head = compound_head(page);
> if (hugepage_migration_supported(page_hstate(head)) &&
> page_huge_active(head))
> return pfn;
> skip = (1 << compound_order(head)) - (page - head);
> pfn += skip - 1;
> }
> return 0;
> }
>
> --- a/mm/memory_hotplug.c~mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix
> +++ a/mm/memory_hotplug.c
> @@ -1305,28 +1305,27 @@ int test_pages_in_a_zone(unsigned long s
> static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
> {
> unsigned long pfn;
> - struct page *page;
> +
> for (pfn = start; pfn < end; pfn++) {
> - if (pfn_valid(pfn)) {
> - page = pfn_to_page(pfn);
> - if (PageLRU(page))
> - return pfn;
> - if (__PageMovable(page))
> - return pfn;
> - if (PageHuge(page)) {
> - struct page *head = compound_head(page);
> + struct page *page, *head;
> + unsigned long skip;
>
> - if (hugepage_migration_supported(page_hstate(head)) &&
> - page_huge_active(head))
> - return pfn;
> - else {
> - unsigned long skip;
> + if (!pfn_valid(pfn))
> + continue;
> + page = pfn_to_page(pfn);
> + if (PageLRU(page))
> + return pfn;
> + if (__PageMovable(page))
> + return pfn;
>
> - skip = (1 << compound_order(head)) - (page - head);
> - pfn += skip - 1;
> - }
> - }
> - }
> + if (!PageHuge(page))
> + continue;
> + head = compound_head(page);
> + if (hugepage_migration_supported(page_hstate(head)) &&
> + page_huge_active(head))
> + return pfn;
> + skip = (1 << compound_order(head)) - (page - head);
> + pfn += skip - 1;
Not sure if encoding the -1 in the previous line is even better now that
we have more space
skip = (1 << compound_order(head)) - (page - head + 1);
Looks good to me.
> }
> return 0;
> }
> _
>
--
Thanks,
David / dhildenb
On Tue, Jan 29, 2019 at 11:03:56AM +0100, David Hildenbrand wrote:
> Not sure if encoding the -1 in the previous line is even better now that
> we have more space
>
> skip = (1 << compound_order(head)) - (page - head + 1);
>
> Looks good to me.
>
> > }
I would rather not do that.
For me looks a bit "subtle" why do we add up 1 after substracting page - head.
I think that doing "skip - 1" looks more clear, but I do not have a strong
opinion here.
--
Oscar Salvador
SUSE L3