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

Groups > linux.kernel > #1538391 > unrolled thread

[PATCH] x86/smpboot: Make logical package management more robust

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/smpboot: Make logical package management more robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-08 10:10 +0100
    Re: [PATCH] x86/smpboot: Make logical package management more robust Borislav Petkov <bp@alien8.de> - 2016-12-08 10:20 +0100
    Re: [PATCH] x86/smpboot: Make logical package management more robust Peter Zijlstra <peterz@infradead.org> - 2016-12-08 13:10 +0100
      Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-08 14:00 +0100
        Re: [PATCH] x86/smpboot: Make logical package management more robust Peter Zijlstra <peterz@infradead.org> - 2016-12-08 14:10 +0100
          Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-08 14:20 +0100
    Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-09 23:10 +0100
      Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-10 00:10 +0100
        Re: [PATCH] x86/smpboot: Make logical package management more robust Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-10 04:40 +0100
          Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-10 20:10 +0100
      Re: [PATCH] x86/smpboot: Make logical package management more robust Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-10 00:10 +0100
        Re: [PATCH] x86/smpboot: Make logical package management more robust Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-10 04:30 +0100
          Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-10 20:20 +0100
            Re: [PATCH] x86/smpboot: Make logical package management more robust Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-11 04:30 +0100
          Re: [PATCH] x86/smpboot: Make logical package management more  robust Thomas Gleixner <tglx@linutronix.de> - 2016-12-10 20:20 +0100

#1538391 — [PATCH] x86/smpboot: Make logical package management more robust

The logical package management has several issues:

 - The APIC ids provided by ACPI are not required to be the same as the
   initial APIC id which can be retrieved by CPUID. The APIC ids provided
   by ACPI are those which are written by the BIOS into the APIC. The
   initial id is set by hardware and can not be changed. The hardware
   provided ids contain the real hardware package information.

   Especially AMD sets the effective APIC id different from the hardware id
   as they need to reserve space for the IOAPIC ids starting at id 0.

   As a consequence those machines trigger the currently active firmware
   bug printouts in dmesg, These are obviously wrong.

 - Virtual machines have their own interesting way of enumerating APICs and
   packages which are not reliably covered by the current implementation.

The sizing of the mapping array has been tweaked to be generously large to
handle systems which provide a wrong core count when HT is disabled so the
whole magic which checks for space in the physical hotplug case is not
needed anymore.

Simplify the whole machinery and do the mapping when the CPU starts and the
CPUID derived physical package information is available. This solves the
observed problems on AMD machines and works for the virtualization issues
as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apic/apic.c  |   15 ------------
 arch/x86/kernel/cpu/common.c |   24 ++++++--------------
 arch/x86/kernel/smpboot.c    |   51 ++++++++++++++++---------------------------
 3 files changed, 27 insertions(+), 63 deletions(-)

--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2159,21 +2159,6 @@ int __generic_processor_info(int apicid,
 	}
 
 	/*
-	 * This can happen on physical hotplug. The sanity check at boot time
-	 * is done from native_smp_prepare_cpus() after num_possible_cpus() is
-	 * established.
-	 */
-	if (topology_update_package_map(apicid, cpu) < 0) {
-		int thiscpu = max + disabled_cpus;
-
-		pr_warning("APIC: Package limit reached. Processor %d/0x%x ignored.\n",
-			   thiscpu, apicid);
-
-		disabled_cpus++;
-		return -ENOSPC;
-	}
-
-	/*
 	 * Validate version
 	 */
 	if (version == 0x0) {
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -979,29 +979,21 @@ static void x86_init_cache_qos(struct cp
 }
 
 /*
- * The physical to logical package id mapping is initialized from the
- * acpi/mptables information. Make sure that CPUID actually agrees with
- * that.
+ * Validate that ACPI/mptables have the same information about the
+ * effective APIC id and update the package map.
  */
-static void sanitize_package_id(struct cpuinfo_x86 *c)
+static void validate_apic_and_package_id(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
-	unsigned int pkg, apicid, cpu = smp_processor_id();
+	unsigned int apicid, cpu = smp_processor_id();
 
 	apicid = apic->cpu_present_to_apicid(cpu);
-	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
 
-	if (apicid != c->initial_apicid) {
-		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID: %x\n",
+	if (apicid != c->apicid) {
+		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x APIC: %x\n",
 		       cpu, apicid, c->initial_apicid);
-		c->initial_apicid = apicid;
 	}
-	if (pkg != c->phys_proc_id) {
-		pr_err(FW_BUG "CPU%u: Using firmware package id %u instead of %u\n",
-		       cpu, pkg, c->phys_proc_id);
-		c->phys_proc_id = pkg;
-	}
-	c->logical_proc_id = topology_phys_to_logical_pkg(pkg);
+	BUG_ON(topology_update_package_map(c->phys_proc_id, cpu));
 #else
 	c->logical_proc_id = 0;
 #endif
@@ -1132,7 +1124,6 @@ static void identify_cpu(struct cpuinfo_
 #ifdef CONFIG_NUMA
 	numa_add_cpu(smp_processor_id());
 #endif
-	sanitize_package_id(c);
 }
 
 /*
@@ -1188,6 +1179,7 @@ void identify_secondary_cpu(struct cpuin
 	enable_sep_cpu();
 #endif
 	mtrr_ap_init();
+	validate_apic_and_package_id(c);
 }
 
 static __init int setup_noclflush(char *arg)
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -104,7 +104,6 @@ static unsigned int max_physical_pkg_id
 unsigned int __max_logical_packages __read_mostly;
 EXPORT_SYMBOL(__max_logical_packages);
 static unsigned int logical_packages __read_mostly;
-static bool logical_packages_frozen __read_mostly;
 
 /* Maximum number of SMT threads on any online core */
 int __max_smt_threads __read_mostly;
@@ -274,9 +273,14 @@ static void notrace start_secondary(void
 	cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
 }
 
-int topology_update_package_map(unsigned int apicid, unsigned int cpu)
+/**
+ * topology_update_package_map - Update the physical to logical package map
+ * @pkg:	The physical package id as retrieved via CPUID
+ * @cpu:	The cpu for which this is updated
+ */
+int topology_update_package_map(unsigned int pkg, unsigned int cpu)
 {
-	unsigned int new, pkg = apicid >> boot_cpu_data.x86_coreid_bits;
+	unsigned int new;
 
 	/* Called from early boot ? */
 	if (!physical_package_map)
@@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
 	if (test_and_set_bit(pkg, physical_package_map))
 		goto found;
 
-	if (logical_packages_frozen) {
-		physical_to_logical_pkg[pkg] = -1;
-		pr_warn("APIC(%x) Package %u exceeds logical package max\n",
-			apicid, pkg);
+	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;
 	}
 
 	new = logical_packages++;
-	pr_info("APIC(%x) Converting physical %u to logical package %u\n",
-		apicid, pkg, new);
+	if (new != pkg) {
+		pr_info("CPU %u Converting physical %u to logical package %u\n",
+			cpu, pkg, new);
+	}
 	physical_to_logical_pkg[pkg] = new;
 
 found:
@@ -319,9 +324,9 @@ int topology_phys_to_logical_pkg(unsigne
 }
 EXPORT_SYMBOL(topology_phys_to_logical_pkg);
 
-static void __init smp_init_package_map(void)
+static void __init smp_init_package_map(unsigned int cpu, unsigned int pkg)
 {
-	unsigned int ncpus, cpu;
+	unsigned int ncpus;
 	size_t size;
 
 	/*
@@ -366,27 +371,9 @@ static void __init smp_init_package_map(
 	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);
-		logical_packages_frozen = true;
-		__max_logical_packages  = logical_packages;
-	}
-
 	pr_info("Max logical packages: %u\n", __max_logical_packages);
+
+	topology_update_package_map(pkg, cpu);
 }
 
 void __init smp_store_boot_cpu_info(void)
@@ -396,7 +383,7 @@ void __init smp_store_boot_cpu_info(void
 
 	*c = boot_cpu_data;
 	c->cpu_index = id;
-	smp_init_package_map();
+	smp_init_package_map(id, c->phys_proc_id);
 }
 
 /*

[toc] | [next] | [standalone]

#1538395

On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> The logical package management has several issues:
> 
>  - The APIC ids provided by ACPI are not required to be the same as the
>    initial APIC id which can be retrieved by CPUID. The APIC ids provided
>    by ACPI are those which are written by the BIOS into the APIC. The
>    initial id is set by hardware and can not be changed. The hardware
>    provided ids contain the real hardware package information.
> 
>    Especially AMD sets the effective APIC id different from the hardware id
>    as they need to reserve space for the IOAPIC ids starting at id 0.
> 
>    As a consequence those machines trigger the currently active firmware
>    bug printouts in dmesg, These are obviously wrong.
> 
>  - Virtual machines have their own interesting way of enumerating APICs and
>    packages which are not reliably covered by the current implementation.
> 
> The sizing of the mapping array has been tweaked to be generously large to
> handle systems which provide a wrong core count when HT is disabled so the
> whole magic which checks for space in the physical hotplug case is not
> needed anymore.
> 
> Simplify the whole machinery and do the mapping when the CPU starts and the
> CPUID derived physical package information is available. This solves the
> observed problems on AMD machines and works for the virtualization issues
> as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Already

Tested-by: Borislav Petkov <bp@suse.de>

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

#1538469

On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> @@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
>  	if (test_and_set_bit(pkg, physical_package_map))
>  		goto found;
>  
> -	if (logical_packages_frozen) {
> -		physical_to_logical_pkg[pkg] = -1;
> -		pr_warn("APIC(%x) Package %u exceeds logical package max\n",
> -			apicid, pkg);
> +	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;
>  	}
>  
>  	new = logical_packages++;
> -	pr_info("APIC(%x) Converting physical %u to logical package %u\n",
> -		apicid, pkg, new);
> +	if (new != pkg) {
> +		pr_info("CPU %u Converting physical %u to logical package %u\n",
> +			cpu, pkg, new);
> +	}

This makes the print conditional on the phy<->logical mapping not
matching; I thought it was a concious decision to print everything in
the initial version.

This way, if we have a 4 node system and nodes 1,2 are crossed we'll
only see:

 "Converting physical 2 to logical package 1"
 "Converting physical 1 to logical package 2"

And nothing on the other two nodes, which could be slightly confusing.

>  	physical_to_logical_pkg[pkg] = new;
>  
>  found:



> @@ -366,27 +371,9 @@ static void __init smp_init_package_map(
>  	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);
> -		logical_packages_frozen = true;
> -		__max_logical_packages  = logical_packages;

So we'll never 'shrink' the initially computed max; which could result
in using more memory than strictly needed, otoh it makes physical
hotplug happier.

> -	}
> -
>  	pr_info("Max logical packages: %u\n", __max_logical_packages);
> +
> +	topology_update_package_map(pkg, cpu);
>  }

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

#1538526 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 10:04:25AM +0100, Thomas Gleixner wrote:
> > @@ -289,16 +293,17 @@ int topology_update_package_map(unsigned
> >  	if (test_and_set_bit(pkg, physical_package_map))
> >  		goto found;
> >  
> > -	if (logical_packages_frozen) {
> > -		physical_to_logical_pkg[pkg] = -1;
> > -		pr_warn("APIC(%x) Package %u exceeds logical package max\n",
> > -			apicid, pkg);
> > +	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;
> >  	}
> >  
> >  	new = logical_packages++;
> > -	pr_info("APIC(%x) Converting physical %u to logical package %u\n",
> > -		apicid, pkg, new);
> > +	if (new != pkg) {
> > +		pr_info("CPU %u Converting physical %u to logical package %u\n",
> > +			cpu, pkg, new);
> > +	}
> 
> This makes the print conditional on the phy<->logical mapping not
> matching; I thought it was a concious decision to print everything in
> the initial version.
> 
> This way, if we have a 4 node system and nodes 1,2 are crossed we'll
> only see:
> 
>  "Converting physical 2 to logical package 1"
>  "Converting physical 1 to logical package 2"
> 
> And nothing on the other two nodes, which could be slightly confusing.

Fair enough. I make it unconditional again.

> > -	if (logical_packages > __max_logical_packages) {
> > -		pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > -			logical_packages, __max_logical_packages);
> > -		logical_packages_frozen = true;
> > -		__max_logical_packages  = logical_packages;
> 
> So we'll never 'shrink' the initially computed max; which could result
> in using more memory than strictly needed, otoh it makes physical
> hotplug happier.

Yes. I was debating that back and forth and at the end decided that making
it simple and robust is a good tradeoff vs. the slightly higher memory
consumption. Though on most systems that's a non issue as number of
possible cpus/packages is the same as the actual available ones. The insane
setups have to suffer - rightfully so.

Thanks,

	tglx

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

#1538535

On Thu, Dec 08, 2016 at 01:49:28PM +0100, Thomas Gleixner wrote:
> 
> > > -	if (logical_packages > __max_logical_packages) {
> > > -		pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > > -			logical_packages, __max_logical_packages);
> > > -		logical_packages_frozen = true;
> > > -		__max_logical_packages  = logical_packages;
> > 
> > So we'll never 'shrink' the initially computed max; which could result
> > in using more memory than strictly needed, otoh it makes physical
> > hotplug happier.
> 
> Yes. I was debating that back and forth and at the end decided that making
> it simple and robust is a good tradeoff vs. the slightly higher memory
> consumption. Though on most systems that's a non issue as number of
> possible cpus/packages is the same as the actual available ones. The insane
> setups have to suffer - rightfully so.

Don't we overestimate by a factor of 2 due to HT? That is, every single
socket Intel box will have a max_packages of 2.

Not that I care too deeply, and arguably the HT case _is_ insane because
its impossible to tell etc..

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

#1538538 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 01:49:28PM +0100, Thomas Gleixner wrote:
> > 
> > > > -	if (logical_packages > __max_logical_packages) {
> > > > -		pr_warn("Detected more packages (%u), then computed by BIOS data (%u).\n",
> > > > -			logical_packages, __max_logical_packages);
> > > > -		logical_packages_frozen = true;
> > > > -		__max_logical_packages  = logical_packages;
> > > 
> > > So we'll never 'shrink' the initially computed max; which could result
> > > in using more memory than strictly needed, otoh it makes physical
> > > hotplug happier.
> > 
> > Yes. I was debating that back and forth and at the end decided that making
> > it simple and robust is a good tradeoff vs. the slightly higher memory
> > consumption. Though on most systems that's a non issue as number of
> > possible cpus/packages is the same as the actual available ones. The insane
> > setups have to suffer - rightfully so.
> 
> Don't we overestimate by a factor of 2 due to HT? That is, every single
> socket Intel box will have a max_packages of 2.
> 
> Not that I care too deeply, and arguably the HT case _is_ insane because
> its impossible to tell etc..

It is insane.

And we can be smart about it for the normal, non physical hotplug case when
all available CPUs are brought up in smp_init() which is _before_ any of
the package users is initialized.

At that point we know exactly how many packages are available and we can
limit max packages to that value. Hmm?

Thanks,

	tglx

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

#1539707 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Thu, 8 Dec 2016, Thomas Gleixner wrote:

Boris, can you please verify if that makes the
topology_update_package_map() call which you placed into the Xen cpu
starting code obsolete ?

Thanks,

	tglx

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

#1539724 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> >
> > Boris, can you please verify if that makes the
> > topology_update_package_map() call which you placed into the Xen cpu
> > starting code obsolete ?
> 
> Will do. I did test your patch but without removing
> topology_update_package_map() call. It complained about package IDs
> being wrong, but that's expected until I fix Xen part.

That should not longer be the case as I changed the approach to that
management thing.

Thanks,

	tglx

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

#1539764

On 12/09/2016 06:00 PM, Thomas Gleixner wrote:
> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
>> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>>
>>> Boris, can you please verify if that makes the
>>> topology_update_package_map() call which you placed into the Xen cpu
>>> starting code obsolete ?
>>
>> Will do. I did test your patch but without removing
>> topology_update_package_map() call. It complained about package IDs
>> being wrong, but that's expected until I fix Xen part.
>
> That should not longer be the case as I changed the approach to that
> management thing.


I didn't notice this email before I sent the earlier message.

Is these anything else besides this patch that I should use? I applied 
it to Linus tree and it didn't apply cleanly (there was some fuzz and 
such) so I wonder whether I am missing something.

-boris

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

#1539892 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 06:00 PM, Thomas Gleixner wrote:
> > On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> > > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > > > 
> > > > Boris, can you please verify if that makes the
> > > > topology_update_package_map() call which you placed into the Xen cpu
> > > > starting code obsolete ?
> > > 
> > > Will do. I did test your patch but without removing
> > > topology_update_package_map() call. It complained about package IDs
> > > being wrong, but that's expected until I fix Xen part.
> > 
> > That should not longer be the case as I changed the approach to that
> > management thing.
> 
> 
> I didn't notice this email before I sent the earlier message.
> 
> Is these anything else besides this patch that I should use? I applied it to
> Linus tree and it didn't apply cleanly (there was some fuzz and such) so I
> wonder whether I am missing something.

No. I did it against tip, but there is nothing which it depends on.

Thanks,

	tglx

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

#1539725

On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>
> Boris, can you please verify if that makes the
> topology_update_package_map() call which you placed into the Xen cpu
> starting code obsolete ?

Will do. I did test your patch but without removing
topology_update_package_map() call. It complained about package IDs
being wrong, but that's expected until I fix Xen part.

-boris

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

#1539763

On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>
>> Boris, can you please verify if that makes the
>> topology_update_package_map() call which you placed into the Xen cpu
>> starting code obsolete ?
>
> Will do. I did test your patch but without removing
> topology_update_package_map() call. It complained about package IDs
> being wrong, but that's expected until I fix Xen part.

Ignore my statement about earlier testing --- it was all on single-node 
machines.

Something is broken with multi-node on Intel, but failure modes are 
different. Prior to this patch build_sched_domain() reports an error and 
pretty soon we crash in scheduler (don't remember off the top of my 
head). With patch applied I crash mush later, when one of the drivers 
does kmalloc_node(.., cpu_to_node(cpu)) and cpu_to_node() returns 1, 
which should never happen ("x86: Booted up 1 node, 32 CPUs" is reported, 
for example).

2-node AMD box doesn't have these problems.

I haven't upgraded the Intel machine for about a month but this all must 
have happened in 4.9 timeframe.

So I can't answer your question since we clearly have other problems on 
Xen. I will be looking into this.

-boris

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

#1539893 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Sat, 10 Dec 2016, Thomas Gleixner wrote:
> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> > On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> > > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > > > 
> > > > Boris, can you please verify if that makes the
> > > > topology_update_package_map() call which you placed into the Xen cpu
> > > > starting code obsolete ?
> > > 
> > > Will do. I did test your patch but without removing
> > > topology_update_package_map() call. It complained about package IDs
> > > being wrong, but that's expected until I fix Xen part.
> > 
> > Ignore my statement about earlier testing --- it was all on single-node
> > machines.
> > 
> > Something is broken with multi-node on Intel, but failure modes are different.
> > Prior to this patch build_sched_domain() reports an error and pretty soon we
> > crash in scheduler (don't remember off the top of my head). With patch applied
> > I crash mush later, when one of the drivers does kmalloc_node(..,
> > cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
> > ("x86: Booted up 1 node, 32 CPUs" is reported, for example).
> 
> Hmm. But the cpu_to_node() association is unrelated to the logical package
> management.

Just came to my mind after hitting send. We had the whole persistent cpuid
to nodeid association work merged in 4.9. So that might be related.

Thanks,

	tglx

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

#1539961

On 12/10/2016 02:13 PM, Thomas Gleixner wrote:
> On Sat, 10 Dec 2016, Thomas Gleixner wrote:
>> On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
>>> On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
>>>> On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
>>>>> On Thu, 8 Dec 2016, Thomas Gleixner wrote:
>>>>>
>>>>> Boris, can you please verify if that makes the
>>>>> topology_update_package_map() call which you placed into the Xen cpu
>>>>> starting code obsolete ?
>>>>
>>>> Will do. I did test your patch but without removing
>>>> topology_update_package_map() call. It complained about package IDs
>>>> being wrong, but that's expected until I fix Xen part.
>>>
>>> Ignore my statement about earlier testing --- it was all on single-node
>>> machines.
>>>
>>> Something is broken with multi-node on Intel, but failure modes are different.
>>> Prior to this patch build_sched_domain() reports an error and pretty soon we
>>> crash in scheduler (don't remember off the top of my head). With patch applied
>>> I crash mush later, when one of the drivers does kmalloc_node(..,
>>> cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
>>> ("x86: Booted up 1 node, 32 CPUs" is reported, for example).
>>
>> Hmm. But the cpu_to_node() association is unrelated to the logical package
>> management.
>
> Just came to my mind after hitting send. We had the whole persistent cpuid
> to nodeid association work merged in 4.9. So that might be related.


Yes, that's exactly the reason.

It uses _PXM to set nodeID and _PXM is exposed to dom0 (which is a 
privileged PV guest).

Re: you previous message: after I "fix" the problem above,  I see
     pr_info("Max logical packages: %u\n", __max_logical_packages);
but no
     pr_warn(CPU %u Converting physical %u to logical package %u\n", ...)

with or without topology_update_package_map() in 
arch/x86/xen/smp.c:cpu_bringup()


-boris

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

#1539894 — Re: [PATCH] x86/smpboot: Make logical package management more robust

On Fri, 9 Dec 2016, Boris Ostrovsky wrote:
> On 12/09/2016 06:02 PM, Boris Ostrovsky wrote:
> > On 12/09/2016 05:06 PM, Thomas Gleixner wrote:
> > > On Thu, 8 Dec 2016, Thomas Gleixner wrote:
> > > 
> > > Boris, can you please verify if that makes the
> > > topology_update_package_map() call which you placed into the Xen cpu
> > > starting code obsolete ?
> > 
> > Will do. I did test your patch but without removing
> > topology_update_package_map() call. It complained about package IDs
> > being wrong, but that's expected until I fix Xen part.
> 
> Ignore my statement about earlier testing --- it was all on single-node
> machines.
> 
> Something is broken with multi-node on Intel, but failure modes are different.
> Prior to this patch build_sched_domain() reports an error and pretty soon we
> crash in scheduler (don't remember off the top of my head). With patch applied
> I crash mush later, when one of the drivers does kmalloc_node(..,
> cpu_to_node(cpu)) and cpu_to_node() returns 1, which should never happen
> ("x86: Booted up 1 node, 32 CPUs" is reported, for example).

Hmm. But the cpu_to_node() association is unrelated to the logical package
management.

> 2-node AMD box doesn't have these problems.
> 
> I haven't upgraded the Intel machine for about a month but this all must have
> happened in 4.9 timeframe.
> 
> So I can't answer your question since we clearly have other problems on Xen. I
> will be looking into this.

Fair enough. What you could do though with this patch applied and the extra
XEN call to topology_update_package_map() removed is to watchout for the
following messages:

  pr_info("Max logical packages: %u\n", __max_logical_packages);

and

  pr_warn(CPU %u Converting physical %u to logical package %u\n", ...)

Ideally the latter wont show.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web