* Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8
@ 2016-05-09 18:39 Josef Bacik
2016-05-09 19:48 ` Thomas Gleixner
2016-05-10 7:33 ` [tip:x86/urgent] x86/topology: Set x86_max_cores to 1 for CONFIG_SMP=n tip-bot for Thomas Gleixner
0 siblings, 2 replies; 5+ messages in thread
From: Josef Bacik @ 2016-05-09 18:39 UTC (permalink / raw)
To: tglx; +Cc: LKML, kernel-team
Hello,
I've hit a regression that was introduced by the commit in $SUBJECT. I'm
building a minimal kernel config that doesn't have CONFIG_SMP set, which
results in the topology for the box to look different than with
CONFIG_SMP set. Specifically boot_cpu_data.x86_max_cores comes out to 16
without CONFIG_SMP set, but 12 with CONFIG_SMP set. So when we are doing
the init for the uncore_msr_uncores types we panic when trying to do
uncore_box_init on the 13th box for hswep_uncore_cbox when doing the
wrmsrl in snbep_uncore_msr_init_box(). I'm hoping this makes sense to
you, because this is all greek to me. For now I've just enabled
CONFIG_SMP (it only adds like .1 mb to my image), but I figure we should
probably fix this. It takes me no time to reproduce so I can test
whatever patch you come up with, or run whatever debugging you want me
to run. Thanks,
Josef
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8
2016-05-09 18:39 Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8 Josef Bacik
@ 2016-05-09 19:48 ` Thomas Gleixner
2016-05-09 20:55 ` Liang, Kan
2016-05-10 6:51 ` Peter Zijlstra
2016-05-10 7:33 ` [tip:x86/urgent] x86/topology: Set x86_max_cores to 1 for CONFIG_SMP=n tip-bot for Thomas Gleixner
1 sibling, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2016-05-09 19:48 UTC (permalink / raw)
To: Josef Bacik; +Cc: LKML, kernel-team, x86, Peter Zijlstra, Kan Liang
Josef,
On Mon, 9 May 2016, Josef Bacik wrote:
> I've hit a regression that was introduced by the commit in $SUBJECT. I'm
> building a minimal kernel config that doesn't have CONFIG_SMP set, which
> results in the topology for the box to look different than with CONFIG_SMP
> set. Specifically boot_cpu_data.x86_max_cores comes out to 16 without
> CONFIG_SMP set, but 12 with CONFIG_SMP set.
So the commit is just exposing the underlying wreckage.
x86_max_cores is set via:
detect_extended_topology(), which is a NOOP in case of SMP=n
or
if the above is not available, which is the case for SMP=n, then it
uses:
intel_num_cpu_cores()
That's using the cache leaf Bits 26-31:
Maximum number of addressable IDs for processor cores in the physical
package
And that's 16 for this CPU, but that has nothing to do with the actual
number of cores in the package.
So that explains the wreckage you are seing. We have two options to deal with
this:
1) Make intel_num_cpu_cores() a NOOP for SMP=n, so x86_max_cores = 1
2) Make detect_extended_topology() functional for SMP=n, so the real number of
cores is detected
Both options work and make sense.
Though I don't know whether the uncore stuff wants to see all boxes of a
package even in the SMP=n case. Kan?
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8
2016-05-09 19:48 ` Thomas Gleixner
@ 2016-05-09 20:55 ` Liang, Kan
2016-05-10 6:51 ` Peter Zijlstra
1 sibling, 0 replies; 5+ messages in thread
From: Liang, Kan @ 2016-05-09 20:55 UTC (permalink / raw)
To: Thomas Gleixner, Josef Bacik; +Cc: LKML, kernel-team, x86, Peter Zijlstra
> Josef,
>
> On Mon, 9 May 2016, Josef Bacik wrote:
>
> > I've hit a regression that was introduced by the commit in $SUBJECT.
> > I'm building a minimal kernel config that doesn't have CONFIG_SMP set,
> > which results in the topology for the box to look different than with
> > CONFIG_SMP set. Specifically boot_cpu_data.x86_max_cores comes out to
> > 16 without CONFIG_SMP set, but 12 with CONFIG_SMP set.
>
> So the commit is just exposing the underlying wreckage.
>
> x86_max_cores is set via:
>
> detect_extended_topology(), which is a NOOP in case of SMP=n
>
> or
>
> if the above is not available, which is the case for SMP=n, then it
> uses:
>
> intel_num_cpu_cores()
>
> That's using the cache leaf Bits 26-31:
>
> Maximum number of addressable IDs for processor cores in the physical
> package
>
> And that's 16 for this CPU, but that has nothing to do with the actual
> number of cores in the package.
>
> So that explains the wreckage you are seing. We have two options to deal
> with
> this:
>
> 1) Make intel_num_cpu_cores() a NOOP for SMP=n, so x86_max_cores = 1
>
> 2) Make detect_extended_topology() functional for SMP=n, so the real
> number of
> cores is detected
>
> Both options work and make sense.
>
> Though I don't know whether the uncore stuff wants to see all boxes of a
> package even in the SMP=n case. Kan?
I think the boxes num should not be larger than the max cores.
So if SMP=n, there is only one core. The boxes num should be one as well.
Option one looks good to me.
Thanks,
Kan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8
2016-05-09 19:48 ` Thomas Gleixner
2016-05-09 20:55 ` Liang, Kan
@ 2016-05-10 6:51 ` Peter Zijlstra
1 sibling, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2016-05-10 6:51 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Josef Bacik, LKML, kernel-team, x86, Kan Liang
On Mon, May 09, 2016 at 09:48:18PM +0200, Thomas Gleixner wrote:
> So that explains the wreckage you are seing. We have two options to deal with
> this:
>
> 1) Make intel_num_cpu_cores() a NOOP for SMP=n, so x86_max_cores = 1
This I think; because:
>
> 2) Make detect_extended_topology() functional for SMP=n, so the real number of
> cores is detected
UP has no business 'knowing' anything about cores/threads and general
topology stuff.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:x86/urgent] x86/topology: Set x86_max_cores to 1 for CONFIG_SMP=n
2016-05-09 18:39 Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8 Josef Bacik
2016-05-09 19:48 ` Thomas Gleixner
@ 2016-05-10 7:33 ` tip-bot for Thomas Gleixner
1 sibling, 0 replies; 5+ messages in thread
From: tip-bot for Thomas Gleixner @ 2016-05-10 7:33 UTC (permalink / raw)
To: linux-tip-commits
Cc: Kernel-team, tglx, mingo, linux-kernel, hpa, peterz, kan.liang, jbacik
Commit-ID: 8d415ee225a3d15d3e3029524350e8237a4de7b8
Gitweb: http://git.kernel.org/tip/8d415ee225a3d15d3e3029524350e8237a4de7b8
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 10 May 2016 09:20:33 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 10 May 2016 09:28:31 +0200
x86/topology: Set x86_max_cores to 1 for CONFIG_SMP=n
Josef reported that the uncore driver trips over with CONFIG_SMP=n because
x86_max_cores is 16 instead of 12.
The reason is, that for SMP=n the extended topology detection is a NOOP and
the cache leaf is used to determine the number of cores. That's wrong in two
aspects:
1) The cache leaf enumerates the maximum addressable number of cores in the
package, which is obviously not correct
2) UP has no business with topology bits at all.
Make intel_num_cpu_cores() return 1 for CONFIG_SMP=n
Reported-by: Josef Bacik <jbacik@fb.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: kernel-team <Kernel-team@fb.com>
Cc: Kan Liang <kan.liang@intel.com>
Link: http://lkml.kernel.org/r/761b4a2a-0332-7954-f030-c6639f949612@fb.com
---
arch/x86/kernel/cpu/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 1f7fdb9..e4393bf 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -336,7 +336,7 @@ static int intel_num_cpu_cores(struct cpuinfo_x86 *c)
{
unsigned int eax, ebx, ecx, edx;
- if (c->cpuid_level < 4)
+ if (!IS_ENABLED(CONFIG_SMP) || c->cpuid_level < 4)
return 1;
/* Intel has a non-standard dependency on %ecx for this CPUID level. */
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-10 7:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-09 18:39 Regression introduced by cf6d445f68974d0b15a14cf6021be38a91f2b5d8 Josef Bacik
2016-05-09 19:48 ` Thomas Gleixner
2016-05-09 20:55 ` Liang, Kan
2016-05-10 6:51 ` Peter Zijlstra
2016-05-10 7:33 ` [tip:x86/urgent] x86/topology: Set x86_max_cores to 1 for CONFIG_SMP=n tip-bot for Thomas Gleixner
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).