Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499725 > unrolled thread

[PATCH] arch/x86: Remove second call to topology_update_package_map()

Back to article view | Back to linux.kernel

Contents

  [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava <prarit@redhat.com> - 2016-10-12 16:40 +0200
    Re: [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava <prarit@redhat.com> - 2016-10-12 17:10 +0200
      Re: [PATCH] arch/x86: Remove second call to  topology_update_package_map() Tim Chen <tim.c.chen@linux.intel.com> - 2016-10-12 22:00 +0200
        Re: [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava <prarit@redhat.com> - 2016-10-12 23:00 +0200
          Re: [PATCH] arch/x86: Remove second call to  topology_update_package_map() Tim Chen <tim.c.chen@linux.intel.com> - 2016-10-13 00:30 +0200
            Re: [PATCH] arch/x86: Remove second call to topology_update_package_map() Prarit Bhargava <prarit@redhat.com> - 2016-10-13 00:40 +0200

#1499725 — [PATCH] 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 <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

[toc] | [next] | [standalone]

#1499753

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();
>  
> 

[toc] | [prev] | [next] | [standalone]

#1499905 — Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()

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

[toc] | [prev] | [next] | [standalone]

#1499918

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
> 

[toc] | [prev] | [next] | [standalone]

#1499940 — Re: [PATCH] arch/x86: Remove second call to topology_update_package_map()

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

[toc] | [prev] | [next] | [standalone]

#1499944

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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web