* [PATCH] arch/x86: Remove second call to topology_update_package_map()
@ 2016-10-12 14:36 Prarit Bhargava
2016-10-12 15:06 ` Prarit Bhargava
0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2016-10-12 14:36 UTC (permalink / raw)
To: linux-kernel
Cc: Prarit Bhargava, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, Peter Zijlstra, Len Brown, Borislav Petkov, Tim Chen,
Andi Kleen, Boris Ostrovsky, Jiri Olsa
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 <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: Len Brown <len.brown@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Jiri Olsa <jolsa@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();
--
1.7.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()
2016-10-12 14:36 [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava
@ 2016-10-12 15:06 ` Prarit Bhargava
2016-10-12 19:51 ` Tim Chen
0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2016-10-12 15:06 UTC (permalink / raw)
To: linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Peter Zijlstra,
Len Brown, Borislav Petkov, Tim Chen, Andi Kleen, Boris Ostrovsky,
Jiri Olsa
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 <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: Len Brown <len.brown@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Jiri Olsa <jolsa@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();
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()
2016-10-12 15:06 ` Prarit Bhargava
@ 2016-10-12 19:51 ` Tim Chen
2016-10-12 20:54 ` Prarit Bhargava
0 siblings, 1 reply; 6+ messages in thread
From: Tim Chen @ 2016-10-12 19:51 UTC (permalink / raw)
To: Prarit Bhargava, linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Peter Zijlstra,
Len Brown, Borislav Petkov, Andi Kleen, Boris Ostrovsky,
Jiri Olsa
On Wed, 2016-10-12 at 11:06 -0400, Prarit Bhargava wrote:
>
> 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.
My single node workstation also failed to boot with the patch.
Tim
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()
2016-10-12 19:51 ` Tim Chen
@ 2016-10-12 20:54 ` Prarit Bhargava
2016-10-12 22:06 ` Tim Chen
0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2016-10-12 20:54 UTC (permalink / raw)
To: Tim Chen, linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Peter Zijlstra,
Len Brown, Borislav Petkov, Andi Kleen, Boris Ostrovsky,
Jiri Olsa
On 10/12/2016 03:51 PM, Tim Chen wrote:
> On Wed, 2016-10-12 at 11:06 -0400, Prarit Bhargava wrote:
>>
>> 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.
>
> My single node workstation also failed to boot with the patch.
The patch is incorrect. I'm dropping it completely.
P.
>
> Tim
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()
2016-10-12 20:54 ` Prarit Bhargava
@ 2016-10-12 22:06 ` Tim Chen
2016-10-12 22:37 ` Prarit Bhargava
0 siblings, 1 reply; 6+ messages in thread
From: Tim Chen @ 2016-10-12 22:06 UTC (permalink / raw)
To: Prarit Bhargava, linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Peter Zijlstra,
Len Brown, Borislav Petkov, Andi Kleen, Boris Ostrovsky,
Jiri Olsa
On Wed, 2016-10-12 at 16:54 -0400, Prarit Bhargava wrote:
>
> On 10/12/2016 03:51 PM, Tim Chen wrote:
> >
> > On Wed, 2016-10-12 at 11:06 -0400, Prarit Bhargava wrote:
> > >
> > >
> > > 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.
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()
2016-10-12 22:06 ` Tim Chen
@ 2016-10-12 22:37 ` Prarit Bhargava
0 siblings, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2016-10-12 22:37 UTC (permalink / raw)
To: Tim Chen, linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Peter Zijlstra,
Len Brown, Borislav Petkov, Andi Kleen, Boris Ostrovsky,
Jiri Olsa
On 10/12/2016 06:06 PM, Tim Chen wrote:
> On Wed, 2016-10-12 at 16:54 -0400, Prarit Bhargava wrote:
>>
>> On 10/12/2016 03:51 PM, Tim Chen wrote:
>>>
>>> On Wed, 2016-10-12 at 11:06 -0400, Prarit Bhargava wrote:
>>>>
>>>>
>>>> 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.
>>> 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.
Right -- the patch is incorrect (see above). I'm dropping it completely.
P.
>
> Tim
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-12 22:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-12 14:36 [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava
2016-10-12 15:06 ` Prarit Bhargava
2016-10-12 19:51 ` Tim Chen
2016-10-12 20:54 ` Prarit Bhargava
2016-10-12 22:06 ` Tim Chen
2016-10-12 22:37 ` Prarit Bhargava
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).