On 10/12/2016 10:36 AM, Prarit Bhargava wrote:
> 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().
>
Self-NAK.
This seems to not work on systems with an empty socket :/. Looking into this
now.
P.
> Signed-off-by: Prarit Bhargava <pra...@redhat.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: "H. Peter Anvin" <h...@zytor.com>
> Cc: x...@kernel.org
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Len Brown <len.br...@intel.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Tim Chen <tim.c.c...@linux.intel.com>
> Cc: Andi Kleen <a...@linux.intel.com>
> Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Cc: Jiri Olsa <jo...@redhat.com>
> ---
> arch/x86/kernel/smpboot.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 951f093a96fe..0ff74f8bb5e9 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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();
>
>
