[PATCH 0/3 v3] x86/smpboot: Fix panic in __max_logical_packages estimate

[PATCH 0/3 v3] x86/smpboot: Fix panic in __max_logical_packages estimate

[Prarit Bhargava]

A system booted with a small number of cores enabled per package
panics because the estimate of __max_logical_packages is too low.
This occurs when the total number of active cores across all packages
is less than the maximum core count for a single package.

Andi noted that the logical_packages calculation wastes 128k on 64-bit
x86 systems and posted patches to fix the allocation.  I have put my
fix on top of those patches.

Here's an example of the panic:

     smpboot: Booting Node   1, Processors  #1 OK
     smpboot: Package 1 of CPU 1 exceeds BIOS package data 1.
     ------------[ cut here ]------------
     kernel BUG at arch/x86/kernel/cpu/common.c:1087!
     invalid opcode: 0000 [#1] SMP
     Modules linked in:
     CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.14.0-rc2+ #4

This was tested by dropping the number of active cores across
all packages to reproduce the bug above.  Additional testing included
2 socket and 4 socket systems and hotplugging entire sockets in different
order.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Piotr Luc <piotr.luc@intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: He Chen <he.chen@linux.intel.com>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>

Andi Kleen (2):
  perf/x86/intel/uncore: Cache logical pkg id in uncore driver
  x86/topology: Avoid wasting 128k for package id array

Prarit Bhargava (1):
  x86/smpboot: Fix __max_logical_packages estimate

 arch/x86/events/intel/uncore.c       |   1 +
 arch/x86/events/intel/uncore.h       |   1 +
 arch/x86/events/intel/uncore_snbep.c |   2 +-
 arch/x86/include/asm/processor.h     |   6 +-
 arch/x86/kernel/smpboot.c            | 124 +++++++++++++----------------------
 5 files changed, 53 insertions(+), 81 deletions(-)

-- 
2.15.0.rc0.39.g2f0e14e64

[PATCH 1/3 v3] perf/x86/intel/uncore: Cache logical pkg id in uncore driver

[Prarit Bhargava]

From: Andi Kleen <ak@linux.intel.com>

The SNB-EP uncore driver is the only user of topology_phys_to_logical_pkg
in a performance critical path. Change it query the logical pkg ID
only once at initialization time and then cache it in box structure.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Piotr Luc <piotr.luc@intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: He Chen <he.chen@linux.intel.com>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/events/intel/uncore.c       | 1 +
 arch/x86/events/intel/uncore.h       | 1 +
 arch/x86/events/intel/uncore_snbep.c | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index d45e06346f14..87ef77982248 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -947,6 +947,7 @@ static int uncore_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id
 
 	atomic_inc(&box->refcnt);
 	box->pci_phys_id = phys_id;
+	box->logical_pkg_id = topology_phys_to_logical_pkg(box->pci_phys_id);
 	box->pkgid = pkg;
 	box->pci_dev = pdev;
 	box->pmu = pmu;
diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index df5989f27b1b..6214cd33a9fb 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -99,6 +99,7 @@ struct intel_uncore_extra_reg {
 
 struct intel_uncore_box {
 	int pci_phys_id;
+	int logical_pkg_id;
 	int pkgid;
 	int n_active;	/* number of active events */
 	int n_events;
diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index a7196818416a..252d371b488b 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -1056,7 +1056,7 @@ static void snbep_qpi_enable_event(struct intel_uncore_box *box, struct perf_eve
 
 	if (reg1->idx != EXTRA_REG_NONE) {
 		int idx = box->pmu->pmu_idx + SNBEP_PCI_QPI_PORT0_FILTER;
-		int pkg = topology_phys_to_logical_pkg(box->pci_phys_id);
+		int pkg = box->logical_pkg_id;
 		struct pci_dev *filter_pdev = uncore_extra_pci_dev[pkg].dev[idx];
 
 		if (filter_pdev) {
-- 
2.15.0.rc0.39.g2f0e14e64

[PATCH 2/3 v3] x86/topology: Avoid wasting 128k for package id array

[Prarit Bhargava]

From: Andi Kleen <ak@linux.intel.com>

I was looking at large early boot allocations and noticed that
since (1f12e32f x86/topology: Create logical package id)
every 64bit system allocates a 128k array to convert logical
package ids.

This happens because the array is sized for
MAX_LOCAL_APIC and that is always 32k on 64bit systems,
and it needs 4 bytes for each entry.

This is fairly wasteful, especially for the common case
of having only one socket, where we need 128K just to store
a single 4 byte value.

The max logical APIC value is not known at this point,
so it's hard to size it correctly.

The previous patch converted the only performance critical
user to cache the value, and all others are fairly
slow path, so we can just convert the O(1) array
lookup to a linear search in cpu_data()

This can also avoid the need for an extra bitmap structure
to know if the logical package ID is already allocated.
We can also save this information in cpu_data and look it
up during the search.

This patch removes the explicit arrays and replaces
the lookups with explicit searches.

Overall the new code is somewhat simpler, and needs a lot
less run time memory.

The naming of the variables in cpu_data is still not
great (_proc sometimes means package and sometimes means
logical processor), but I followed the existing (messy)
conventions when possible. At some point would be probably good
to clean this up too.

Tested on a 2S system, but it would be good
to test on more obscure systems which may have problems
with package IDs. I'm copying Prarit who had problematic systems
before.

[v2]: Decrease logical_packages when the last thread in a socket is
removed.
[v3]: Add more logic to keep logical and physical package IDs
in synch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Piotr Luc <piotr.luc@intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: He Chen <he.chen@linux.intel.com>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/processor.h |  6 +++-
 arch/x86/kernel/smpboot.c        | 77 +++++++++++++++++++++-------------------
 2 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b390ff76e58f..51aef8d3bb58 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -124,13 +124,17 @@ struct cpuinfo_x86 {
 	u16			booted_cores;
 	/* Physical processor id: */
 	u16			phys_proc_id;
-	/* Logical processor id: */
+	/* Logical processor (package) id: */
 	u16			logical_proc_id;
+	/* Physical package ID */
+	u16			phys_pkg_id;
 	/* Core id: */
 	u16			cpu_core_id;
 	/* Index into per_cpu list: */
 	u16			cpu_index;
 	u32			microcode;
+	/* Flags */
+	unsigned		logical_proc_set : 1;
 } __randomize_layout;
 
 struct cpuid_regs {
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ad59edd84de7..d0f44b4b806f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -100,9 +100,6 @@ DEFINE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
 
 /* Logical package management. We might want to allocate that dynamically */
-static int *physical_to_logical_pkg __read_mostly;
-static unsigned long *physical_package_map __read_mostly;;
-static unsigned int max_physical_pkg_id __read_mostly;
 unsigned int __max_logical_packages __read_mostly;
 EXPORT_SYMBOL(__max_logical_packages);
 static unsigned int logical_packages __read_mostly;
@@ -285,17 +282,11 @@ static void notrace start_secondary(void *unused)
  */
 int topology_update_package_map(unsigned int pkg, unsigned int cpu)
 {
-	unsigned int new;
+	int new;
 
-	/* Called from early boot ? */
-	if (!physical_package_map)
-		return 0;
-
-	if (pkg >= max_physical_pkg_id)
-		return -EINVAL;
-
-	/* Set the logical package id */
-	if (test_and_set_bit(pkg, physical_package_map))
+	/* Has this package been mapped to a logical package? */
+	new = topology_phys_to_logical_pkg(pkg);
+	if (new >= 0)
 		goto found;
 
 	if (logical_packages >= __max_logical_packages) {
@@ -304,15 +295,27 @@ int topology_update_package_map(unsigned int pkg, unsigned int cpu)
 		return -ENOSPC;
 	}
 
-	new = logical_packages++;
-	if (new != pkg) {
-		pr_info("CPU %u Converting physical %u to logical package %u\n",
-			cpu, pkg, new);
+	logical_packages++;
+
+	/* Try to keep 1:1 physical to logical mapping */
+	if (pkg <= logical_packages) {
+		new = pkg;
+		goto found;
 	}
-	physical_to_logical_pkg[pkg] = new;
 
+	/* Find an unused logical mapping value */
+	new = 0;
+retry:
+	if (topology_phys_to_logical_pkg(new) >= 0) {
+			new++;
+			goto retry;
+	}
+	pr_info("CPU %u Converting physical %u to logical package %u\n",
+		cpu, pkg, new);
 found:
-	cpu_data(cpu).logical_proc_id = physical_to_logical_pkg[pkg];
+	cpu_data(cpu).phys_pkg_id = pkg;
+	cpu_data(cpu).logical_proc_id = new;
+	cpu_data(cpu).logical_proc_set = 1;
 	return 0;
 }
 
@@ -323,16 +326,21 @@ int topology_update_package_map(unsigned int pkg, unsigned int cpu)
  */
 int topology_phys_to_logical_pkg(unsigned int phys_pkg)
 {
-	if (phys_pkg >= max_physical_pkg_id)
-		return -1;
-	return physical_to_logical_pkg[phys_pkg];
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu_data(cpu).phys_pkg_id == phys_pkg &&
+		    cpu_data(cpu).logical_proc_set) {
+			return cpu_data(cpu).logical_proc_id;
+		}
+	}
+	return -1;
 }
 EXPORT_SYMBOL(topology_phys_to_logical_pkg);
 
 static void __init smp_init_package_map(struct cpuinfo_x86 *c, unsigned int cpu)
 {
 	unsigned int ncpus;
-	size_t size;
 
 	/*
 	 * Today neither Intel nor AMD support heterogenous systems. That
@@ -363,21 +371,10 @@ static void __init smp_init_package_map(struct cpuinfo_x86 *c, unsigned int cpu)
 	}
 
 	__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
-	 * package can be smaller than the actual used apic ids.
-	 */
-	max_physical_pkg_id = DIV_ROUND_UP(MAX_LOCAL_APIC, ncpus);
-	size = max_physical_pkg_id * sizeof(unsigned int);
-	physical_to_logical_pkg = kmalloc(size, GFP_KERNEL);
-	memset(physical_to_logical_pkg, 0xff, size);
-	size = BITS_TO_LONGS(max_physical_pkg_id) * sizeof(unsigned long);
-	physical_package_map = kzalloc(size, GFP_KERNEL);
-
 	pr_info("Max logical packages: %u\n", __max_logical_packages);
 
+	logical_packages = 0;
+
 	topology_update_package_map(c->phys_proc_id, cpu);
 }
 
@@ -1508,7 +1505,7 @@ static void recompute_smt_state(void)
 
 static void remove_siblinginfo(int cpu)
 {
-	int sibling;
+	int phys_pkg_id, sibling;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
 	for_each_cpu(sibling, topology_core_cpumask(cpu)) {
@@ -1529,6 +1526,12 @@ static void remove_siblinginfo(int cpu)
 	cpumask_clear(topology_core_cpumask(cpu));
 	c->phys_proc_id = 0;
 	c->cpu_core_id = 0;
+
+	phys_pkg_id = c->phys_pkg_id;
+	c->phys_pkg_id = U16_MAX;
+	if (topology_phys_to_logical_pkg(phys_pkg_id) < 0)
+		logical_packages--;
+
 	cpumask_clear_cpu(cpu, cpu_sibling_setup_mask);
 	recompute_smt_state();
 }
-- 
2.15.0.rc0.39.g2f0e14e64

[PATCH 3/3 v3] x86/smpboot: Fix __max_logical_packages estimate

[Prarit Bhargava]

A system booted with a small number of cores enabled per package
panics because the estimate of __max_logical_packages is too low.
This occurs when the total number of active cores across all packages
is less than the maximum core count for a single package.

ie) On a 4 package system with 20 cores/package where only 4 cores
are enabled on each package, the value of __max_logical_packages is
calculated as DIV_ROUND_UP(16 / 20) = 1 and not 4.

Here's an example of the panic:

     smpboot: Booting Node   1, Processors  #1 OK
     smpboot: Package 1 of CPU 1 exceeds BIOS package data 1.
     ------------[ cut here ]------------
     kernel BUG at arch/x86/kernel/cpu/common.c:1087!
     invalid opcode: 0000 [#1] SMP
     Modules linked in:
     CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.14.0-rc2+ #4

Calculate __max_logical_packages after the cpu enumeration has completed.
Use the boot cpu's data to extrapolate the number of packages.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Piotr Luc <piotr.luc@intel.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: He Chen <he.chen@linux.intel.com>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kernel/smpboot.c | 57 +++++++++--------------------------------------
 1 file changed, 10 insertions(+), 47 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index d0f44b4b806f..83cdd7f3c835 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -289,12 +289,6 @@ int topology_update_package_map(unsigned int pkg, unsigned int cpu)
 	if (new >= 0)
 		goto found;
 
-	if (logical_packages >= __max_logical_packages) {
-		pr_warn("Package %u of CPU %u exceeds BIOS package data %u.\n",
-			logical_packages, cpu, __max_logical_packages);
-		return -ENOSPC;
-	}
-
 	logical_packages++;
 
 	/* Try to keep 1:1 physical to logical mapping */
@@ -338,46 +332,6 @@ int topology_phys_to_logical_pkg(unsigned int phys_pkg)
 }
 EXPORT_SYMBOL(topology_phys_to_logical_pkg);
 
-static void __init smp_init_package_map(struct cpuinfo_x86 *c, unsigned int cpu)
-{
-	unsigned int ncpus;
-
-	/*
-	 * Today neither Intel nor AMD support heterogenous systems. That
-	 * might change in the future....
-	 *
-	 * While ideally we'd want '* smp_num_siblings' in the below @ncpus
-	 * computation, this won't actually work since some Intel BIOSes
-	 * report inconsistent HT data when they disable HT.
-	 *
-	 * In particular, they reduce the APIC-IDs to only include the cores,
-	 * but leave the CPUID topology to say there are (2) siblings.
-	 * This means we don't know how many threads there will be until
-	 * after the APIC enumeration.
-	 *
-	 * By not including this we'll sometimes over-estimate the number of
-	 * logical packages by the amount of !present siblings, but this is
-	 * still better than MAX_LOCAL_APIC.
-	 *
-	 * We use total_cpus not nr_cpu_ids because nr_cpu_ids can be limited
-	 * on the command line leading to a similar issue as the HT disable
-	 * problem because the hyperthreads are usually enumerated after the
-	 * primary cores.
-	 */
-	ncpus = boot_cpu_data.x86_max_cores;
-	if (!ncpus) {
-		pr_warn("x86_max_cores == zero !?!?");
-		ncpus = 1;
-	}
-
-	__max_logical_packages = DIV_ROUND_UP(total_cpus, ncpus);
-	pr_info("Max logical packages: %u\n", __max_logical_packages);
-
-	logical_packages = 0;
-
-	topology_update_package_map(c->phys_proc_id, cpu);
-}
-
 void __init smp_store_boot_cpu_info(void)
 {
 	int id = 0; /* CPU 0 */
@@ -385,7 +339,7 @@ void __init smp_store_boot_cpu_info(void)
 
 	*c = boot_cpu_data;
 	c->cpu_index = id;
-	smp_init_package_map(c, id);
+	topology_update_package_map(c->phys_proc_id, id);
 }
 
 /*
@@ -1385,7 +1339,16 @@ void __init native_smp_prepare_boot_cpu(void)
 
 void __init native_smp_cpus_done(unsigned int max_cpus)
 {
+	int ncpus;
+
 	pr_debug("Boot done\n");
+	/*
+	 * Today neither Intel nor AMD support heterogenous systems so
+	 * extrapolate the boot cpu's data to all packages.
+	 */
+	ncpus = cpu_data(0).booted_cores * smp_num_siblings;
+	__max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
+	pr_info("Max logical packages: %u\n", __max_logical_packages);
 
 	if (x86_has_numa_in_package)
 		set_sched_topology(x86_numa_in_package_topology);
-- 
2.15.0.rc0.39.g2f0e14e64

Re: [PATCH 2/3 v3] x86/topology: Avoid wasting 128k for package id array

[Thomas Gleixner]

On Thu, 19 Oct 2017, Prarit Bhargava wrote:
>  static void remove_siblinginfo(int cpu)
>  {
> -	int sibling;
> +	int phys_pkg_id, sibling;
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  
>  	for_each_cpu(sibling, topology_core_cpumask(cpu)) {
> @@ -1529,6 +1526,12 @@ static void remove_siblinginfo(int cpu)
>  	cpumask_clear(topology_core_cpumask(cpu));
>  	c->phys_proc_id = 0;
>  	c->cpu_core_id = 0;
> +
> +	phys_pkg_id = c->phys_pkg_id;
> +	c->phys_pkg_id = U16_MAX;

This leaves c->logical_proc_set = 1, which is inconsistent at best. I have
no idea why we need this logical_proc_set flag at all.

> +	if (topology_phys_to_logical_pkg(phys_pkg_id) < 0)
> +		logical_packages--;

Now this has another issue. Depending on hotplug ordering the logical
package association can change across hotplug operations. I don't know it
that's an issue, but this needs to be analyzed before we merge that.

Thanks,

	tglx

Re: [PATCH 2/3 v3] x86/topology: Avoid wasting 128k for package id array

[Prarit Bhargava]

On 10/20/2017 05:03 AM, Thomas Gleixner wrote:
> On Thu, 19 Oct 2017, Prarit Bhargava wrote:
>>  static void remove_siblinginfo(int cpu)
>>  {
>> -	int sibling;
>> +	int phys_pkg_id, sibling;
>>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>>  
>>  	for_each_cpu(sibling, topology_core_cpumask(cpu)) {
>> @@ -1529,6 +1526,12 @@ static void remove_siblinginfo(int cpu)
>>  	cpumask_clear(topology_core_cpumask(cpu));
>>  	c->phys_proc_id = 0;
>>  	c->cpu_core_id = 0;
>> +
>> +	phys_pkg_id = c->phys_pkg_id;
>> +	c->phys_pkg_id = U16_MAX;
> 
> This leaves c->logical_proc_set = 1, which is inconsistent at best. I have
> no idea why we need this logical_proc_set flag at all.
> 
>> +	if (topology_phys_to_logical_pkg(phys_pkg_id) < 0)
>> +		logical_packages--;
> 
> Now this has another issue. Depending on hotplug ordering the logical
> package association can change across hotplug operations. I don't know it
> that's an issue, but this needs to be analyzed before we merge that.
> 

Thanks for making me look at this Thomas.

Andi, it looks like this is unfortunately an issue.  I have a reworked
patchset that fixes by dynamically allocating a u16
logical_to_physical_package array that

a)  maps the logical to physical package IDs,
b) is dynamically sized to the value of logical_packages, and,
c) is of size logical_packages (not MAX_APICS)

This will resolve the problem that Thomas has pointed out, and it
would address the issue of wasting memory.

I'm waiting for a 4S & 8S system to test the LTP hotplug tests on
and then I'll post.  Andi, I'll send it to you in private email for
a quick review.

P.

> Thanks,
> 
> 	tglx
> 
>