perf/x86/intel/uncore: Initialize with correct logical package ID
Subsystems (2)
PATCH 1/1 perf/x86/intel/uncore: Initialize with correct logical package ID
On multi-socket Intel v3 processor systems (aka Haswell) kdump can fail with:
BUG: unable to handle kernel paging request at 00000000006563a1 IP: [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 PGD 0 [ 2.313897] Oops: 0000 [#1] SMP Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0 #1 Hardware name: NEC Express5800/T120f [N8100-2285Y]/GA-7WESV-NJ, BIOS 5.0.4009 08/01/2016 task: ffff88002bdb8000 task.stack: ffffc90000014000 RIP: 0010:[<ffffffff8101b582>] [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 RSP: 0000:ffffc90000017db8 EFLAGS: 00010206 RAX: 0000000000656369 RBX: 0000000000000000 RCX: 0000000000001e03 RDX: ffff88002b224780 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffffc90000017dc8 R08: 000000000001c880 R09: ffffffff813667e1 R10: ffff880030c1c880 R11: 0000000000000000 R12: 0000000000000000 R13: ffffffff81c1c090 R14: afafafafafafafaf R15: afafafafafafafaf FS: 0000000000000000(0000) GS:ffff880030c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000006563a1 CR3: 000000002fc07000 CR4: 00000000001406b0 Stack: ffffc90000017dc8 00000000352bd002 ffffc90000017e00 ffffffff81da17f8 0000000000000000 ffffffff81da16f9 00000000000000f0 afafafafafafafaf afafafafafafafaf ffffc90000017e78 ffffffff81002190 ffffc90000017e00 Call Trace: [<ffffffff81da17f8>] intel_uncore_init+0xff/0x2e6 [<ffffffff81da16f9>] ? uncore_type_init+0x158/0x158 [<ffffffff81002190>] do_one_initcall+0x50/0x190 [<ffffffff810af27b>] ? parse_args+0x27b/0x460 [<ffffffff81d9c357>] kernel_init_freeable+0x1a5/0x249 [<ffffffff81d9ba27>] ? set_debug_rodata+0x12/0x12 [<ffffffff81702010>] ? rest_init+0x80/0x80 [<ffffffff8170201e>] kernel_init+0xe/0x110 [<ffffffff8170f715>] ret_from_fork+0x25/0x30 Code: 1a d5 00 39 15 cc 1c c0 00 7e 06 89 15 c4 1c c0 00 48 98 48 8b 15 d7 c3 f7 00 48 8d 04 40 48 8d 04 c2 48 8b 40 10 48 85 c0 74 1b <8b> 70 38 48 8b 78 10 48 8d 4d f4 ba 94 00 00 00 e8 b9 db 38 00 RIP [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0
This is now occuring because 9d85eb9119f4 ("x86/smpboot: Make logical package management more robust") corrected the physical ID to logical ID mapping of the threads. hswep_uncore_cpu_init() is hard coded for physical socket 0 and if the system is kdump'ing on any other socket the logical package value will be incorrect. The code should not use 0 as the physical ID, and should use the boot cpu's physical package ID in this calculation.
--- arch/x86/events/intel/uncore_snbep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/events/intel/uncore_snbep.c +1 -1
@@ -2686,7 +2686,7 @@ static int hswep_pcu_hw_config(struct intel_uncore_box *box, struct perf_event *void hswep_uncore_cpu_init (void ) { -int pkg =topology_phys_to_logical_pkg (0 ); +int pkg =topology_phys_to_logical_pkg (boot_cpu_data.phys_proc_id);if (hswep_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores) hswep_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
Signature · 1 line
1.7.9.3
Quoted 62 lines · Prarit Bhargava
On multi-socket Intel v3 processor systems (aka Haswell) kdump can fail with:
BUG: unable to handle kernel paging request at 00000000006563a1 IP: [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 PGD 0 [ 2.313897] Oops: 0000 [#1] SMP Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0 #1 Hardware name: NEC Express5800/T120f [N8100-2285Y]/GA-7WESV-NJ, BIOS 5.0.4009 08/01/2016 task: ffff88002bdb8000 task.stack: ffffc90000014000 RIP: 0010:[<ffffffff8101b582>] [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 RSP: 0000:ffffc90000017db8 EFLAGS: 00010206 RAX: 0000000000656369 RBX: 0000000000000000 RCX: 0000000000001e03 RDX: ffff88002b224780 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffffc90000017dc8 R08: 000000000001c880 R09: ffffffff813667e1 R10: ffff880030c1c880 R11: 0000000000000000 R12: 0000000000000000 R13: ffffffff81c1c090 R14: afafafafafafafaf R15: afafafafafafafaf FS: 0000000000000000(0000) GS:ffff880030c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000006563a1 CR3: 000000002fc07000 CR4: 00000000001406b0 Stack: ffffc90000017dc8 00000000352bd002 ffffc90000017e00 ffffffff81da17f8 0000000000000000 ffffffff81da16f9 00000000000000f0 afafafafafafafaf afafafafafafafaf ffffc90000017e78 ffffffff81002190 ffffc90000017e00 Call Trace: [<ffffffff81da17f8>] intel_uncore_init+0xff/0x2e6 [<ffffffff81da16f9>] ? uncore_type_init+0x158/0x158 [<ffffffff81002190>] do_one_initcall+0x50/0x190 [<ffffffff810af27b>] ? parse_args+0x27b/0x460 [<ffffffff81d9c357>] kernel_init_freeable+0x1a5/0x249 [<ffffffff81d9ba27>] ? set_debug_rodata+0x12/0x12 [<ffffffff81702010>] ? rest_init+0x80/0x80 [<ffffffff8170201e>] kernel_init+0xe/0x110 [<ffffffff8170f715>] ret_from_fork+0x25/0x30 Code: 1a d5 00 39 15 cc 1c c0 00 7e 06 89 15 c4 1c c0 00 48 98 48 8b 15 d7 c3 f7 00 48 8d 04 40 48 8d 04 c2 48 8b 40 10 48 85 c0 74 1b <8b> 70 38 48 8b 78 10 48 8d 4d f4 ba 94 00 00 00 e8 b9 db 38 00 RIP [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0This is now occuring because
9d85eb9119f4("x86/smpboot: Make logical package management more robust") corrected the physical ID to logical ID mapping of the threads.hswep_uncore_cpu_init()is hard coded for physical socket 0 and if the system is kdump'ing on any other socket the logical package value will be incorrect. The code should not use 0 as the physical ID, and should use the boot cpu's physical package ID in this calculation.Signed-off-by Prarit Bhargava <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Kan Liang <[email protected]>Cc Borislav Petkov <[email protected]>Cc Harish Chegondi <[email protected]>--- arch/x86/events/intel/uncore_snbep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/events/intel/uncore_snbep.c +0
-0
arch/x86/events/intel/uncore_snbep.c static int hswep_pcu_hw_config(struct intel_uncore_box *box, struct perf_event * · line 2689
Quoted 5 lines
arch/x86/events/intel/uncore_snbep.c +1
-1@@ -2686,7 +2686,7 @@ static int hswep_pcu_hw_config(struct intel_uncore_box *box, struct perf_event *void hswep_uncore_cpu_init (void ) { -int pkg =topology_phys_to_logical_pkg (0 ); +int pkg =topology_phys_to_logical_pkg (boot_cpu_data.phys_proc_id);
One thing that just occurred to me as I was looking at other code. boot_cpu_data has logical_proc_id, so it may be better to use that instead of the lookup function.
I'm not sure of the usage of physical_to_logical_pkg[] and logical_proc_id. Unless tglx or someone already knows of a reason not to use logical_proc_id I certainly can change the patch.
P.
Quoted 4 lines
arch/x86/events/intel/uncore_snbep.c +0
-0if (hswep_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores) hswep_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
Quoted 28 lines · Prarit Bhargava
On multi-socket Intel v3 processor systems (aka Haswell) kdump can fail with:
BUG: unable to handle kernel paging request at 00000000006563a1 IP: [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 PGD 0 [ 2.313897] Oops: 0000 [#1] SMP Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0 #1 Hardware name: NEC Express5800/T120f [N8100-2285Y]/GA-7WESV-NJ, BIOS 5.0.4009 08/01/2016 task: ffff88002bdb8000 task.stack: ffffc90000014000 RIP: 0010:[<ffffffff8101b582>] [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0 RSP: 0000:ffffc90000017db8 EFLAGS: 00010206 RAX: 0000000000656369 RBX: 0000000000000000 RCX: 0000000000001e03 RDX: ffff88002b224780 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffffc90000017dc8 R08: 000000000001c880 R09: ffffffff813667e1 R10: ffff880030c1c880 R11: 0000000000000000 R12: 0000000000000000 R13: ffffffff81c1c090 R14: afafafafafafafaf R15: afafafafafafafaf FS: 0000000000000000(0000) GS:ffff880030c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000006563a1 CR3: 000000002fc07000 CR4: 00000000001406b0 Stack: ffffc90000017dc8 00000000352bd002 ffffc90000017e00 ffffffff81da17f8 0000000000000000 ffffffff81da16f9 00000000000000f0 afafafafafafafaf afafafafafafafaf ffffc90000017e78 ffffffff81002190 ffffc90000017e00 Call Trace: [<ffffffff81da17f8>] intel_uncore_init+0xff/0x2e6 [<ffffffff81da16f9>] ? uncore_type_init+0x158/0x158 [<ffffffff81002190>] do_one_initcall+0x50/0x190
Quoted 8 lines
[<ffffffff810af27b>] ? parse_args+0x27b/0x460 [<ffffffff81d9c357>] kernel_init_freeable+0x1a5/0x249 [<ffffffff81d9ba27>] ? set_debug_rodata+0x12/0x12 [<ffffffff81702010>] ? rest_init+0x80/0x80 [<ffffffff8170201e>] kernel_init+0xe/0x110 [<ffffffff8170f715>] ret_from_fork+0x25/0x30 Code: 1a d5 00 39 15 cc 1c c0 00 7e 06 89 15 c4 1c c0 00 48 98 48 8b 15 d7 c3 f7 00 48 8d 04 40 48 8d 04 c2 48 8b 40 10 48 85 c0 74 1b <8b> 70 38 48 8b 78 10 48 8d 4d f4 ba 94 00 00 00 e8 b9 db 38 00 RIP [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0
And that back trace is useful because it looks good and occupies a lot of space in the changelog? There is no value as we already know the call chain. Back traces are only useful if they show a particular call path of many possible ones and help to explain the problem.
Quoted 3 lines
This is now occuring because
9d85eb9119f4("x86/smpboot: Make logical package management more robust") corrected the physical ID to logical ID mapping of the threads.
That's nonsense. This has absolutely nothing to do with that particular commit. Using the hardcoded 0 physical id is simply wrong since:
commit cf6d445f6897 ("perf/x86/uncore: Track packages, not per CPU data")
Quoted 4 lines
hswep_uncore_cpu_init()is hard coded for physical socket 0 and if the system is kdump'ing on any other socket the logical package value will be incorrect. The code should not use 0 as the physical ID, and should use the boot cpu's physical package ID in this calculation.
Should?
No, it MUST use that. Documentation/process/submitting-patches.rst:
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" ... as if you are giving orders to the codebase to change its behaviour.
I really like your patches, but you might finally start to write real change logs instead of fairy tales.
Thanks,
tglx
tip-bot for Prarit Bhargava
merge bot[tip:x86/urgent] perf/x86/intel/uncore: Do not use hard coded physical package id 0
In reply to Prarit Bhargavaperf/x86/intel/uncore: Do not use hard coded physical package id 0
Commit-ID: 42433049c51e326baa1f45c834af9572fdb65b35 Gitweb: http://git.kernel.org/tip/42433049c51e326baa1f45c834af9572fdb65b35 Author: Prarit Bhargava <[email protected]> AuthorDate: Tue, 3 Jan 2017 14:24:31 -0500 Committer: Thomas Gleixner <[email protected]> CommitDate: Wed, 11 Jan 2017 11:29:37 +0100
perf/x86/intel/uncore: Do not use hard coded physical package id 0
hswep_uncore_cpu_init() uses a hardcoded physical package id 0 for the boot cpu. This works as long as the boot CPU is actually on the physical package 0, which is normaly the case after power on / reboot.
But it fails with a NULL pointer dereference when a kdump kernel is started on a secondary socket which has a different physical package id because the locigal package translation for physical package 0 does not exist.
Use the physical package id of the boot cpu instead of hard coded 0.
[ tglx: Rewrote changelog once more ]
commit cf6d445f6897 ("perf/x86/uncore: Track packages, not per CPU data")
--- arch/x86/events/intel/uncore_snbep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/events/intel/uncore_snbep.c +1 -1
@@ -2686,7 +2686,7 @@ static struct intel_uncore_type *hswep_msr_uncores[] = {void hswep_uncore_cpu_init (void ) { -int pkg =topology_phys_to_logical_pkg (0 ); +int pkg =topology_phys_to_logical_pkg (boot_cpu_data.phys_proc_id);if (hswep_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores) hswep_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
Re: [tip:x86/urgent] perf/x86/intel/uncore: Do not use hard coded physical package id 0
Quoted 41 lines · tip-bot for Prarit Bhargava
Commit-ID: 42433049c51e326baa1f45c834af9572fdb65b35 Gitweb: http://git.kernel.org/tip/42433049c51e326baa1f45c834af9572fdb65b35 Author: Prarit Bhargava <[email protected]> AuthorDate: Tue, 3 Jan 2017 14:24:31 -0500 Committer: Thomas Gleixner <[email protected]> CommitDate: Wed, 11 Jan 2017 11:29:37 +0100perf/x86/intel/uncore: Do not use hard coded physical package id 0
hswep_uncore_cpu_init()uses a hardcoded physical package id 0 for the boot cpu. This works as long as the boot CPU is actually on the physical package 0, which is normaly the case after power on / reboot.But it fails with a NULL pointer dereference when a kdump kernel is started on a secondary socket which has a different physical package id because the locigal package translation for physical package 0 does not exist.
Use the physical package id of the boot cpu instead of hard coded 0.
[ tglx: Rewrote changelog once more ]
commit
cf6d445f6897("perf/x86/uncore: Track packages, not per CPU data")Signed-off-by Prarit Bhargava <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Kan Liang <[email protected]>Cc Harish Chegondi <[email protected]>Cc Borislav Petkov <[email protected]>Signed-off-by Thomas Gleixner <[email protected]>--- arch/x86/events/intel/uncore_snbep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/events/intel/uncore_snbep.c +0
-0
arch/x86/events/intel/uncore_snbep.c static struct intel_uncore_type *hswep_msr_uncores[] = { · line 2692
Quoted 8 lines
arch/x86/events/intel/uncore_snbep.c +1
-1@@ -2686,7 +2686,7 @@ static struct intel_uncore_type *hswep_msr_uncores[] = {void hswep_uncore_cpu_init (void ) { -int pkg =topology_phys_to_logical_pkg (0 ); +int pkg =topology_phys_to_logical_pkg (boot_cpu_data.phys_proc_id);if (hswep_uncore_cbox.num_boxes > boot_cpu_data.x86_max_cores) hswep_uncore_cbox.num_boxes = boot_cpu_data.x86_max_cores;
Thomas, I self-nacked this and posted a v2. The v2 uses boot_cpu_data.logical_proc_id instead. It was picked up here in tip by Ingo yesterday.
http://git.kernel.org/tip/fa37361e291bfe528872b9aef5c8644a3fc7ff20
P.
Re: [tip:x86/urgent] perf/x86/intel/uncore: Do not use hard coded physical package id 0
Quoted 3 lines · Prarit Bhargava
Thomas, I self-nacked this and posted a v2. The v2 uses boot_cpu_data.logical_proc_id instead.
Which is a cosmetic change.
Quoted 4 lines
It was picked up here in tip by Ingo
yesterday.http://git.kernel.org/tip/fa37361e291bfe528872b9aef5c8644a3fc7ff20
Which contains the same broken changelog and misses a stable tag. I'll fix that up.
Thanks,
tglx