diff mbox series

mm,memory_hotplug: Fix scan_movable_pages for gigantic hugepages

Message ID 20190122154407.18417-1-osalvador@suse.de (mailing list archive)
State New, archived
Headers show
Series mm,memory_hotplug: Fix scan_movable_pages for gigantic hugepages | expand

Commit Message

Oscar Salvador Jan. 22, 2019, 3:44 p.m. UTC
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 <osalvador@suse.de>
---
 mm/memory_hotplug.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Anthony Yznaga Jan. 22, 2019, 11:47 p.m. UTC | #1
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 <osalvador@suse.de>

Looks good.

Reviewed-by: Anthony Yznaga <anthony.yznaga@oracle.com>

> ---
>  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;
> +				}
>  			}
>  		}
>  	}
Michal Hocko Jan. 23, 2019, 9:47 a.m. UTC | #2
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 <osalvador@suse.de>

Other than that the change looks good to me.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  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
Oscar Salvador Jan. 23, 2019, 10:18 a.m. UTC | #3
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 <osalvador@suse.de>
> 
> Other than that the change looks good to me.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
Michal Hocko Jan. 23, 2019, 10:22 a.m. UTC | #4
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 <osalvador@suse.de>
> > 
> > Other than that the change looks good to me.
> > 
> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> -- 
> Oscar Salvador
> SUSE L3
David Hildenbrand Jan. 23, 2019, 10:33 a.m. UTC | #5
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 <osalvador@suse.de>
> ---
>  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 <david@redhat.com>

> +					unsigned long skip;
> +
> +					skip = (1 << compound_order(head)) - (page - head);
> +					pfn += skip - 1;
> +				}
>  			}
>  		}
>  	}
>
Oscar Salvador Jan. 25, 2019, 7:58 a.m. UTC | #6
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 <david@redhat.com>

Thanks David ;-)
Andrew Morton Jan. 28, 2019, 10:53 p.m. UTC | #7
On Tue, 22 Jan 2019 16:44:07 +0100 Oscar Salvador <osalvador@suse.de> 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 <osalvador@suse.de>

And I'll add cc:stable.
Andrew Morton Jan. 28, 2019, 10:53 p.m. UTC | #8
On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <osalvador@suse.de> 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 <akpm@linux-foundation.org>
Subject: mmmemory_hotplug-fix-scan_movable_pages-for-gigantic-hugepages-fix

fix brace layout, per David.  Also reduce indentation

Cc: Anthony Yznaga <anthony.yznaga@oracle.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Oscar Salvador <osalvador@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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;
Andrew Morton Jan. 28, 2019, 10:56 p.m. UTC | #9
On Mon, 28 Jan 2019 14:53:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <osalvador@suse.de> 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;
 }
Michal Hocko Jan. 29, 2019, 7:33 a.m. UTC | #10
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
Oscar Salvador Jan. 29, 2019, 8:27 a.m. UTC | #11
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!
David Hildenbrand Jan. 29, 2019, 10:03 a.m. UTC | #12
On 28.01.19 23:56, Andrew Morton wrote:
> On Mon, 28 Jan 2019 14:53:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Fri, 25 Jan 2019 08:58:33 +0100 Oscar Salvador <osalvador@suse.de> 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;
>  }
> _
>
Oscar Salvador Jan. 30, 2019, 7:52 a.m. UTC | #13
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.
diff mbox series

Patch

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;
+				}
 			}
 		}
 	}