arch/x86: Remove second call to topology_update_package_map()

This was noticed during the investigation of

http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c

A note for reviewers on the cleanup in smp_init_package_map(): The per_cpu variable x86_bios_cpu_apicid is initialized to BAD_APICID, and the bitmaps are set to false by default so the code isn't necessary. The pr_warn() can also be dropped as generic_processor_info() will do a pr_warning() in the same situation.

P.

-----8<-----

After commit 1f12e32f4cd5 ("x86/topology: Create logical package id"), topology_update_package_map() is called twice on a CPU. The first call is in generic_processor_info(), which is called on all present cpus (found in either mptable or ACPI MADT). The second call is in smp_init_package_map() which is called later on in the init sequence.

Only a single call is necessary for the kernel to initialize the data structures properly. This patch drops the later call in smp_init_package_map() and moves smp_init_package_map() out of smp_store_boot_cpu_info().

Signed-off-by Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Cc Len Brown <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tim Chen <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Jiri Olsa <[email protected]>

---
 arch/x86/kernel/smpboot.c |   20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

arch/x86/kernel/smpboot.c +3 -17

index 951f093a96fe..0ff74f8bb5e9 100644
@@ -271,7 +271,7 @@ int topology_update_package_map(unsigned int apicid, unsigned int cpu)
 	if (!physical_package_map)
 		return 0;
 
-	if (pkg >= max_physical_pkg_id)
+	if (!apic->apic_id_valid(apicid) || pkg >= max_physical_pkg_id)
 		return -EINVAL;
 
 	/* Set the logical package id */

@@ -310,7 +310,7 @@ int topology_phys_to_logical_pkg(unsigned int phys_pkg)
 
 static void __init smp_init_package_map(void)
 {
-	unsigned int ncpus, cpu;
+	unsigned int ncpus;
 	size_t size;
 
 	/*

@@ -342,7 +342,6 @@ static void __init smp_init_package_map(void)
 	}
 
 	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
-	logical_packages = 0;
 
 	/*
 	 * Possibly larger than what we need as the number of apic ids per

@@ -355,19 +354,6 @@ static void __init smp_init_package_map(void)
 	size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
 	physical_package_map = kzalloc(size, GFP_KERNEL);
 
-	for_each_present_cpu(cpu) {
-		unsigned int apicid = apic->cpu_present_to_apicid(cpu);
-
-		if (apicid == BAD_APICID || !apic->apic_id_valid(apicid))
-			continue;
-		if (!topology_update_package_map(apicid, cpu))
-			continue;
-		pr_warn("CPU %u APICId %x disabled\n", cpu, apicid);
-		per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID;
-		set_cpu_possible(cpu, false);
-		set_cpu_present(cpu, false);
-	}
-
 	if (logical_packages > __max_logical_packages) {
 		pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
 			logical_packages, __max_logical_packages);

@@ -385,7 +371,6 @@ void __init smp_store_boot_cpu_info(void)
 
 	*c = boot_cpu_data;
 	c->cpu_index = id;
-	smp_init_package_map();
 }
 
 /*

@@ -1285,6 +1270,7 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 	 * Setup boot CPU information
 	 */
 	smp_store_boot_cpu_info(); /* Final full version of the data */
+	smp_init_package_map();
 	cpumask_copy(cpu_callin_mask, cpumask_of(0));
 	mb();
 

Quoted 23 lines · Prarit Bhargava

This was noticed during the investigation of

http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c

A note for reviewers on the cleanup in smp_init_package_map(): The per_cpu variable x86_bios_cpu_apicid is initialized to BAD_APICID, and the bitmaps are set to false by default so the code isn't necessary.  The pr_warn() can also be dropped as generic_processor_info() will do a pr_warning() in the same situation.

P.

-----8<-----

After commit 1f12e32f4cd5 ("x86/topology: Create logical package id"), topology_update_package_map() is called twice on a CPU.  The first call is in generic_processor_info(), which is called on all present cpus (found in either mptable or ACPI MADT).  The second call is in smp_init_package_map() which is called later on in the init sequence.

Quoted 4 lines

Only a single call is necessary for the kernel to initialize the data structures properly. This patch drops the later call in smp_init_package_map() and moves smp_init_package_map() out of smp_store_boot_cpu_info().

Self-NAK.

This seems to not work on systems with an empty socket :/.  Looking into this now.

P.

Quoted 29 lines · Prarit Bhargava

Quoted 27 lines · Prarit Bhargava

This was noticed during the investigation of

http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c

A note for reviewers on the cleanup in smp_init_package_map(): The per_cpu variable x86_bios_cpu_apicid is initialized to BAD_APICID, and the bitmaps are set to false by default so the code isn't necessary. The pr_warn() can also be dropped as generic_processor_info() will do a pr_warning() in the same situation.

P.

-----8<-----

After commit 1f12e32f4cd5 ("x86/topology: Create logical package id"), topology_update_package_map() is called twice on a CPU. The first call is in generic_processor_info(), which is called on all present cpus (found in either mptable or ACPI MADT). The second call is in smp_init_package_map() which is called later on in the init sequence.

Only a single call is necessary for the kernel to initialize the data structures properly. This patch drops the later call in smp_init_package_map() and moves smp_init_package_map() out of smp_store_boot_cpu_info().

Quoted 5 lines

Self-NAK.

This seems to not work on systems with an empty socket :/. Looking into this now.

P.

My single node workstation also failed to boot with the patch.

Tim

Quoted 33 lines · Tim Chen

Quoted 31 lines · Prarit Bhargava

Quoted 28 lines · Prarit Bhargava

This was noticed during the investigation of

http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c

A note for reviewers on the cleanup in smp_init_package_map(): The per_cpu variable x86_bios_cpu_apicid is initialized to BAD_APICID, and the bitmaps are set to false by default so the code isn't necessary.  The pr_warn() can also be dropped as generic_processor_info() will do a pr_warning() in the same situation.

P.

-----8<-----

After commit 1f12e32f4cd5 ("x86/topology: Create logical package id"), topology_update_package_map() is called twice on a CPU.  The first call is in generic_processor_info(), which is called on all present cpus (found in either mptable or ACPI MADT).  The second call is in smp_init_package_map() which is called later on in the init sequence.

Only a single call is necessary for the kernel to initialize the data structures properly. This patch drops the later call in smp_init_package_map() and moves smp_init_package_map() out of smp_store_boot_cpu_info().

Quoted 6 lines

Quoted 5 lines

Self-NAK.

This seems to not work on systems with an empty socket :/.  Looking into this now.

P.

My single node workstation also failed to boot with the patch.

The patch is incorrect.  I'm dropping it completely.

Quoted 35 lines · Prarit Bhargava

Quoted 33 lines · Tim Chen

Quoted 31 lines · Prarit Bhargava

Quoted 28 lines · Prarit Bhargava

This was noticed during the investigation of

http://git.kernel.org/tip/2a51fe083eba7f99cbda72f5ef90cdf2f4df882c

A note for reviewers on the cleanup in smp_init_package_map(): The per_cpu variable x86_bios_cpu_apicid is initialized to BAD_APICID, and the bitmaps are set to false by default so the code isn't necessary. The pr_warn() can also be dropped as generic_processor_info() will do a pr_warning() in the same situation.

P.

-----8<-----

After commit 1f12e32f4cd5 ("x86/topology: Create logical package id"), topology_update_package_map() is called twice on a CPU. The first call is in generic_processor_info(), which is called on all present cpus (found in either mptable or ACPI MADT). The second call is in smp_init_package_map() which is called later on in the init sequence.

Only a single call is necessary for the kernel to initialize the data structures properly. This patch drops the later call in smp_init_package_map() and moves smp_init_package_map() out of smp_store_boot_cpu_info().

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

Self-NAK.

This seems to not work on systems with an empty socket :/. Looking into this now.

P.

My single node workstation also failed to boot with the patch.

The patch is incorrect. I'm dropping it completely.

The call from generic_processor_info to topology_update_package_map during early boot was essentially no op, as physical_package_map has not have been allocated by smp_init_package_map yet.

We have to call topology_update_package_map in smp_init_package_map after allocating physical_package_map to properly set up physical_package_map.

Tim