* [PATCH] sched/x86: construct all sibling maps if smt
@ 2013-05-27 17:09 Andrew Jones
2013-05-29 10:26 ` Andrew Jones
2013-05-29 12:48 ` [PATCH v2] " Andrew Jones
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Jones @ 2013-05-27 17:09 UTC (permalink / raw)
To: tglx, mingo, hpa, x86, a.p.zijlstra, fenghua.yu; +Cc: linux-kernel
Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke
the construction of sibling maps, which also broke the booted_cores
accounting.
Before the rewrite, if smt was present, then each map was updated for
each smt sibling. After the rewrite only cpu_sibling_mask gets updated,
as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead.
This leads to problems with topologies like the following
(qemu -smp sockets=2,cores=1,threads=2)
processor : 0
physical id : 0
siblings : 1 <= should be 2
core id : 0
cpu cores : 1
processor : 1
physical id : 0
siblings : 1 <= should be 2
core id : 0
cpu cores : 0 <= should be 1
processor : 2
physical id : 1
siblings : 1 <= should be 2
core id : 0
cpu cores : 1
processor : 3
physical id : 1
siblings : 1 <= should be 2
core id : 0
cpu cores : 0 <= should be 1
This patch restores the former construction by defining has_mc as
(has_smt || x86_max_cores > 1). This should be fine as there were no
(has_smt && !has_mc) conditions in the context.
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
arch/x86/kernel/smpboot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 9c73b51817e47..886a3234eaff3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
void __cpuinit set_cpu_sibling_map(int cpu)
{
- bool has_mc = boot_cpu_data.x86_max_cores > 1;
bool has_smt = smp_num_siblings > 1;
+ bool has_mc = has_smt || boot_cpu_data.x86_max_cores > 1;
struct cpuinfo_x86 *c = &cpu_data(cpu);
struct cpuinfo_x86 *o;
int i;
cpumask_set_cpu(cpu, cpu_sibling_setup_mask);
- if (!has_smt && !has_mc) {
+ if (!has_mc) {
cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu));
cpumask_set_cpu(cpu, cpu_core_mask(cpu));
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sched/x86: construct all sibling maps if smt 2013-05-27 17:09 [PATCH] sched/x86: construct all sibling maps if smt Andrew Jones @ 2013-05-29 10:26 ` Andrew Jones 2013-05-29 11:06 ` Peter Zijlstra 2013-05-29 12:48 ` [PATCH v2] " Andrew Jones 1 sibling, 1 reply; 6+ messages in thread From: Andrew Jones @ 2013-05-29 10:26 UTC (permalink / raw) To: tglx, mingo, hpa, x86, a.p.zijlstra, fenghua.yu; +Cc: linux-kernel On Mon, May 27, 2013 at 07:09:00PM +0200, Andrew Jones wrote: > Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke > the construction of sibling maps, which also broke the booted_cores > accounting. > > Before the rewrite, if smt was present, then each map was updated for > each smt sibling. After the rewrite only cpu_sibling_mask gets updated, > as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead. > This leads to problems with topologies like the following > > (qemu -smp sockets=2,cores=1,threads=2) > > processor : 0 > physical id : 0 > siblings : 1 <= should be 2 > core id : 0 > cpu cores : 1 > > processor : 1 > physical id : 0 > siblings : 1 <= should be 2 > core id : 0 > cpu cores : 0 <= should be 1 > > processor : 2 > physical id : 1 > siblings : 1 <= should be 2 > core id : 0 > cpu cores : 1 > > processor : 3 > physical id : 1 > siblings : 1 <= should be 2 > core id : 0 > cpu cores : 0 <= should be 1 > > This patch restores the former construction by defining has_mc as > (has_smt || x86_max_cores > 1). This should be fine as there were no > (has_smt && !has_mc) conditions in the context. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > arch/x86/kernel/smpboot.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 9c73b51817e47..886a3234eaff3 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) > > void __cpuinit set_cpu_sibling_map(int cpu) > { > - bool has_mc = boot_cpu_data.x86_max_cores > 1; > bool has_smt = smp_num_siblings > 1; > + bool has_mc = has_smt || boot_cpu_data.x86_max_cores > 1; > struct cpuinfo_x86 *c = &cpu_data(cpu); > struct cpuinfo_x86 *o; > int i; > > cpumask_set_cpu(cpu, cpu_sibling_setup_mask); > > - if (!has_smt && !has_mc) { > + if (!has_mc) { > cpumask_set_cpu(cpu, cpu_sibling_mask(cpu)); > cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu)); > cpumask_set_cpu(cpu, cpu_core_mask(cpu)); > -- > 1.8.1.4 > Any acks? This patch fixes a regression. Also, in case anybody is wondering, this is not the same regression as was already fixed with ceb1cbac8eda6 sched/x86: Calculate booted cores after construction of sibling_mask (Hmm, I probably should have renamed has_mc to has_mp, as the redefinition expands its scope. I'm not sure if that deserves a v2 though.) drew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/x86: construct all sibling maps if smt 2013-05-29 10:26 ` Andrew Jones @ 2013-05-29 11:06 ` Peter Zijlstra 2013-05-29 11:11 ` Ingo Molnar 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2013-05-29 11:06 UTC (permalink / raw) To: Andrew Jones; +Cc: tglx, mingo, hpa, x86, fenghua.yu, linux-kernel On Wed, May 29, 2013 at 12:26:01PM +0200, Andrew Jones wrote: > On Mon, May 27, 2013 at 07:09:00PM +0200, Andrew Jones wrote: > > Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke > > the construction of sibling maps, which also broke the booted_cores > > accounting. > > > > Before the rewrite, if smt was present, then each map was updated for > > each smt sibling. After the rewrite only cpu_sibling_mask gets updated, > > as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead. > > This leads to problems with topologies like the following > > > > (qemu -smp sockets=2,cores=1,threads=2) > > > > processor : 0 > > physical id : 0 > > siblings : 1 <= should be 2 > > core id : 0 > > cpu cores : 1 > > > > processor : 1 > > physical id : 0 > > siblings : 1 <= should be 2 > > core id : 0 > > cpu cores : 0 <= should be 1 > > > > processor : 2 > > physical id : 1 > > siblings : 1 <= should be 2 > > core id : 0 > > cpu cores : 1 > > > > processor : 3 > > physical id : 1 > > siblings : 1 <= should be 2 > > core id : 0 > > cpu cores : 0 <= should be 1 > > > > This patch restores the former construction by defining has_mc as > > (has_smt || x86_max_cores > 1). This should be fine as there were no > > (has_smt && !has_mc) conditions in the context. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > arch/x86/kernel/smpboot.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > > index 9c73b51817e47..886a3234eaff3 100644 > > --- a/arch/x86/kernel/smpboot.c > > +++ b/arch/x86/kernel/smpboot.c > > @@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) > > > > void __cpuinit set_cpu_sibling_map(int cpu) > > { > > - bool has_mc = boot_cpu_data.x86_max_cores > 1; > > bool has_smt = smp_num_siblings > 1; > > + bool has_mc = has_smt || boot_cpu_data.x86_max_cores > 1; > > struct cpuinfo_x86 *c = &cpu_data(cpu); > > struct cpuinfo_x86 *o; > > int i; > > > > cpumask_set_cpu(cpu, cpu_sibling_setup_mask); > > > > - if (!has_smt && !has_mc) { > > + if (!has_mc) { > > cpumask_set_cpu(cpu, cpu_sibling_mask(cpu)); > > cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu)); > > cpumask_set_cpu(cpu, cpu_core_mask(cpu)); > > -- > > 1.8.1.4 > > > > Any acks? This patch fixes a regression. Also, in case anybody is > wondering, this is not the same regression as was already fixed with > > ceb1cbac8eda6 sched/x86: Calculate booted cores after construction of sibling_mask > > (Hmm, I probably should have renamed has_mc to has_mp, as the redefinition > expands its scope. I'm not sure if that deserves a v2 though.) Right, took me a while to bend my brain around that code again -- I obviously don't have the best track record since this is the second bug in it since I rewrote the thing (with the intent of making it 'easier' to read ha!). Yes, I think your patch is correct, and your suggestion of doing s/has_mc/has_mp/ seems a sensible one too. Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/x86: construct all sibling maps if smt 2013-05-29 11:06 ` Peter Zijlstra @ 2013-05-29 11:11 ` Ingo Molnar 0 siblings, 0 replies; 6+ messages in thread From: Ingo Molnar @ 2013-05-29 11:11 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Jones, tglx, mingo, hpa, x86, fenghua.yu, linux-kernel * Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, May 29, 2013 at 12:26:01PM +0200, Andrew Jones wrote: > > On Mon, May 27, 2013 at 07:09:00PM +0200, Andrew Jones wrote: > > > Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke > > > the construction of sibling maps, which also broke the booted_cores > > > accounting. > > > > > > Before the rewrite, if smt was present, then each map was updated for > > > each smt sibling. After the rewrite only cpu_sibling_mask gets updated, > > > as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead. > > > This leads to problems with topologies like the following > > > > > > (qemu -smp sockets=2,cores=1,threads=2) > > > > > > processor : 0 > > > physical id : 0 > > > siblings : 1 <= should be 2 > > > core id : 0 > > > cpu cores : 1 > > > > > > processor : 1 > > > physical id : 0 > > > siblings : 1 <= should be 2 > > > core id : 0 > > > cpu cores : 0 <= should be 1 > > > > > > processor : 2 > > > physical id : 1 > > > siblings : 1 <= should be 2 > > > core id : 0 > > > cpu cores : 1 > > > > > > processor : 3 > > > physical id : 1 > > > siblings : 1 <= should be 2 > > > core id : 0 > > > cpu cores : 0 <= should be 1 > > > > > > This patch restores the former construction by defining has_mc as > > > (has_smt || x86_max_cores > 1). This should be fine as there were no > > > (has_smt && !has_mc) conditions in the context. > > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > > --- > > > arch/x86/kernel/smpboot.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > > > index 9c73b51817e47..886a3234eaff3 100644 > > > --- a/arch/x86/kernel/smpboot.c > > > +++ b/arch/x86/kernel/smpboot.c > > > @@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) > > > > > > void __cpuinit set_cpu_sibling_map(int cpu) > > > { > > > - bool has_mc = boot_cpu_data.x86_max_cores > 1; > > > bool has_smt = smp_num_siblings > 1; > > > + bool has_mc = has_smt || boot_cpu_data.x86_max_cores > 1; > > > struct cpuinfo_x86 *c = &cpu_data(cpu); > > > struct cpuinfo_x86 *o; > > > int i; > > > > > > cpumask_set_cpu(cpu, cpu_sibling_setup_mask); > > > > > > - if (!has_smt && !has_mc) { > > > + if (!has_mc) { > > > cpumask_set_cpu(cpu, cpu_sibling_mask(cpu)); > > > cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu)); > > > cpumask_set_cpu(cpu, cpu_core_mask(cpu)); > > > -- > > > 1.8.1.4 > > > > > > > Any acks? This patch fixes a regression. Also, in case anybody is > > wondering, this is not the same regression as was already fixed with > > > > ceb1cbac8eda6 sched/x86: Calculate booted cores after construction of sibling_mask > > > > (Hmm, I probably should have renamed has_mc to has_mp, as the redefinition > > expands its scope. I'm not sure if that deserves a v2 though.) > > Right, took me a while to bend my brain around that code again -- I > obviously don't have the best track record since this is the second bug > in it since I rewrote the thing (with the intent of making it 'easier' > to read ha!). > > Yes, I think your patch is correct, and your suggestion of doing > s/has_mc/has_mp/ seems a sensible one too. > > Thanks! Andrew, it would be nice to have a -v2 with that rename and with Peter's Acked-by included. Thanks, Ingo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] sched/x86: construct all sibling maps if smt 2013-05-27 17:09 [PATCH] sched/x86: construct all sibling maps if smt Andrew Jones 2013-05-29 10:26 ` Andrew Jones @ 2013-05-29 12:48 ` Andrew Jones 2013-05-31 12:50 ` [tip:sched/urgent] sched/x86: Construct " tip-bot for Andrew Jones 1 sibling, 1 reply; 6+ messages in thread From: Andrew Jones @ 2013-05-29 12:48 UTC (permalink / raw) To: tglx, mingo, hpa, x86, a.p.zijlstra, fenghua.yu; +Cc: linux-kernel Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke the construction of sibling maps, which also broke the booted_cores accounting. Before the rewrite, if smt was present, then each map was updated for each smt sibling. After the rewrite only cpu_sibling_mask gets updated, as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead. This leads to problems with topologies like the following (qemu -smp sockets=2,cores=1,threads=2) processor : 0 physical id : 0 siblings : 1 <= should be 2 core id : 0 cpu cores : 1 processor : 1 physical id : 0 siblings : 1 <= should be 2 core id : 0 cpu cores : 0 <= should be 1 processor : 2 physical id : 1 siblings : 1 <= should be 2 core id : 0 cpu cores : 1 processor : 3 physical id : 1 siblings : 1 <= should be 2 core id : 0 cpu cores : 0 <= should be 1 This patch restores the former construction by defining has_mc as (has_smt || x86_max_cores > 1). This should be fine as there were no (has_smt && !has_mc) conditions in the context. v2: - rename has_mc to has_mp now that it's not just for cores Signed-off-by: Andrew Jones <drjones@redhat.com> Acked-by: Peter Zijlstra <peterz@infradead.org> --- arch/x86/kernel/smpboot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 9c73b51817e47..bfd348e993692 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) void __cpuinit set_cpu_sibling_map(int cpu) { - bool has_mc = boot_cpu_data.x86_max_cores > 1; bool has_smt = smp_num_siblings > 1; + bool has_mp = has_smt || boot_cpu_data.x86_max_cores > 1; struct cpuinfo_x86 *c = &cpu_data(cpu); struct cpuinfo_x86 *o; int i; cpumask_set_cpu(cpu, cpu_sibling_setup_mask); - if (!has_smt && !has_mc) { + if (!has_mp) { cpumask_set_cpu(cpu, cpu_sibling_mask(cpu)); cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu)); cpumask_set_cpu(cpu, cpu_core_mask(cpu)); @@ -394,7 +394,7 @@ void __cpuinit set_cpu_sibling_map(int cpu) if ((i == cpu) || (has_smt && match_smt(c, o))) link_mask(sibling, cpu, i); - if ((i == cpu) || (has_mc && match_llc(c, o))) + if ((i == cpu) || (has_mp && match_llc(c, o))) link_mask(llc_shared, cpu, i); } @@ -406,7 +406,7 @@ void __cpuinit set_cpu_sibling_map(int cpu) for_each_cpu(i, cpu_sibling_setup_mask) { o = &cpu_data(i); - if ((i == cpu) || (has_mc && match_mc(c, o))) { + if ((i == cpu) || (has_mp && match_mc(c, o))) { link_mask(core, cpu, i); /* -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [tip:sched/urgent] sched/x86: Construct all sibling maps if smt 2013-05-29 12:48 ` [PATCH v2] " Andrew Jones @ 2013-05-31 12:50 ` tip-bot for Andrew Jones 0 siblings, 0 replies; 6+ messages in thread From: tip-bot for Andrew Jones @ 2013-05-31 12:50 UTC (permalink / raw) To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, drjones, peterz, tglx Commit-ID: b0bc225d0e5de887340d4d92a8c594ef0f60d412 Gitweb: http://git.kernel.org/tip/b0bc225d0e5de887340d4d92a8c594ef0f60d412 Author: Andrew Jones <drjones@redhat.com> AuthorDate: Wed, 29 May 2013 14:48:15 +0200 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 31 May 2013 13:10:38 +0200 sched/x86: Construct all sibling maps if smt Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()") broke the construction of sibling maps, which also broke the booted_cores accounting. Before the rewrite, if smt was present, then each map was updated for each smt sibling. After the rewrite only cpu_sibling_mask gets updated, as the llc and core maps depend on 'has_mc = x86_max_cores > 1' instead. This leads to problems with topologies like the following (qemu -smp sockets=2,cores=1,threads=2) processor : 0 physical id : 0 siblings : 1 <= should be 2 core id : 0 cpu cores : 1 processor : 1 physical id : 0 siblings : 1 <= should be 2 core id : 0 cpu cores : 0 <= should be 1 processor : 2 physical id : 1 siblings : 1 <= should be 2 core id : 0 cpu cores : 1 processor : 3 physical id : 1 siblings : 1 <= should be 2 core id : 0 cpu cores : 0 <= should be 1 This patch restores the former construction by defining has_mc as (has_smt || x86_max_cores > 1). This should be fine as there were no (has_smt && !has_mc) conditions in the context. Aso rename has_mc to has_mp now that it's not just for cores. Signed-off-by: Andrew Jones <drjones@redhat.com> Acked-by: Peter Zijlstra <peterz@infradead.org> Cc: a.p.zijlstra@chello.nl Cc: fenghua.yu@intel.com Link: http://lkml.kernel.org/r/1369831695-11970-1-git-send-email-drjones@redhat.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/kernel/smpboot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 9c73b51..bfd348e 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -372,15 +372,15 @@ static bool __cpuinit match_mc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) void __cpuinit set_cpu_sibling_map(int cpu) { - bool has_mc = boot_cpu_data.x86_max_cores > 1; bool has_smt = smp_num_siblings > 1; + bool has_mp = has_smt || boot_cpu_data.x86_max_cores > 1; struct cpuinfo_x86 *c = &cpu_data(cpu); struct cpuinfo_x86 *o; int i; cpumask_set_cpu(cpu, cpu_sibling_setup_mask); - if (!has_smt && !has_mc) { + if (!has_mp) { cpumask_set_cpu(cpu, cpu_sibling_mask(cpu)); cpumask_set_cpu(cpu, cpu_llc_shared_mask(cpu)); cpumask_set_cpu(cpu, cpu_core_mask(cpu)); @@ -394,7 +394,7 @@ void __cpuinit set_cpu_sibling_map(int cpu) if ((i == cpu) || (has_smt && match_smt(c, o))) link_mask(sibling, cpu, i); - if ((i == cpu) || (has_mc && match_llc(c, o))) + if ((i == cpu) || (has_mp && match_llc(c, o))) link_mask(llc_shared, cpu, i); } @@ -406,7 +406,7 @@ void __cpuinit set_cpu_sibling_map(int cpu) for_each_cpu(i, cpu_sibling_setup_mask) { o = &cpu_data(i); - if ((i == cpu) || (has_mc && match_mc(c, o))) { + if ((i == cpu) || (has_mp && match_mc(c, o))) { link_mask(core, cpu, i); /* ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-31 12:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-27 17:09 [PATCH] sched/x86: construct all sibling maps if smt Andrew Jones 2013-05-29 10:26 ` Andrew Jones 2013-05-29 11:06 ` Peter Zijlstra 2013-05-29 11:11 ` Ingo Molnar 2013-05-29 12:48 ` [PATCH v2] " Andrew Jones 2013-05-31 12:50 ` [tip:sched/urgent] sched/x86: Construct " tip-bot for Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.