2012-05-31 07:37:47

by Kamalesh Babulal

[permalink] [raw]
Subject: [PATCH] sched/x86: Calculate booted cores after construction of sibling_mask

While booting tip (1d06e6d354) on qemu with -smp sockets=1,cores=2,threads=2
topology. There is a mismatch between the number of cores actually present
and cores reported.

Changes related to calculating the number of booted cores was introduced
by 316ad248307fb (sched/x86: Rewrite set_cpu_sibling_map()). With the
clean up, the calculation of booted cores per package is done on an
incompletely constructed sibling_mask per cpu.

sched/x86: Calculate booted cores after construction of sibling_mask

This patch re-introduces the old behaviour of constructing the complete
sibling mask of the cpu, before calculating the number of cores booted.

On qemu booted with -smp sockets=1,cores=2,threads=2,
Before:
$ cat /proc/cpuinfo |grep cores
cpu cores : 2
cpu cores : 1
cpu cores : 4
cpu cores : 3

With the patch:
$ cat /proc/cpuinfo |grep cores
cpu cores : 2
cpu cores : 2
cpu cores : 2
cpu cores : 2

Signed-off-by: Kamalesh Babulal <[email protected]>
---
arch/x86/kernel/smpboot.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fd019d7..a8d78f3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -382,7 +382,12 @@ void __cpuinit set_cpu_sibling_map(int cpu)
if ((i == cpu) || (has_mc && match_llc(c, o)))
link_mask(llc_shared, cpu, i);

- if ((i == cpu) || (has_mc && match_mc(c, o))) {
+ }
+
+ for_each_cpu(i, cpu_sibling_setup_mask) {
+ o = &cpu_data(i);
+
+ if ((i == cpu) || (has_mc && match_mc(c, o))) {
link_mask(core, cpu, i);

/*


2012-05-31 20:31:11

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] sched/x86: Calculate booted cores after construction of sibling_mask

On Thu, 2012-05-31 at 13:07 +0530, Kamalesh Babulal wrote:
> While booting tip (1d06e6d354) on qemu with -smp sockets=1,cores=2,threads=2
> topology. There is a mismatch between the number of cores actually present
> and cores reported.
>
> Changes related to calculating the number of booted cores was introduced
> by 316ad248307fb (sched/x86: Rewrite set_cpu_sibling_map()). With the
> clean up, the calculation of booted cores per package is done on an
> incompletely constructed sibling_mask per cpu.
>
> sched/x86: Calculate booted cores after construction of sibling_mask

Why repeat this subject again?

> This patch re-introduces the old behaviour of constructing the complete
> sibling mask of the cpu, before calculating the number of cores booted.

> Signed-off-by: Kamalesh Babulal <[email protected]>
> ---
> arch/x86/kernel/smpboot.c | 7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index fd019d7..a8d78f3 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -382,7 +382,12 @@ void __cpuinit set_cpu_sibling_map(int cpu)
> if ((i == cpu) || (has_mc && match_llc(c, o)))
> link_mask(llc_shared, cpu, i);
>
> - if ((i == cpu) || (has_mc && match_mc(c, o))) {
> + }
> +
> + for_each_cpu(i, cpu_sibling_setup_mask) {
> + o = &cpu_data(i);
> +
> + if ((i == cpu) || (has_mc && match_mc(c, o))) {
> link_mask(core, cpu, i);
>
> /*

This patch actually horribly mangles the indenting. I did the below to
it:


---
Subject: sched/x86: Calculate booted cores after construction of sibling_mask
From: Kamalesh Babulal <[email protected]>
Date: Thu, 31 May 2012 13:07:38 +0530

Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()")
broke the booted_cores accounting.

The problem is that the booted_cores accounting needs all the
sibling links set up. So restore the second loop and add a comment as
to why its needed.

On qemu booted with -smp sockets=1,cores=2,threads=2;
Before:
$ grep cores /proc/cpuinfo
cpu cores : 2
cpu cores : 1
cpu cores : 4
cpu cores : 3

With the patch:
$ grep cores /proc/cpuinfo
cpu cores : 2
cpu cores : 2
cpu cores : 2
cpu cores : 2

Signed-off-by: Kamalesh Babulal <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/smpboot.c | 9 +++++++++
1 file changed, 9 insertions(+)

--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -382,6 +382,15 @@ void __cpuinit set_cpu_sibling_map(int c
if ((i == cpu) || (has_mc && match_llc(c, o)))
link_mask(llc_shared, cpu, i);

+ }
+
+ /*
+ * This needs a separate iteration over the cpus because we rely on all
+ * cpu_sibling_mask links to be set-up.
+ */
+ for_each_cpu(i, cpu_sibling_setup_mask) {
+ o = &cpu_data(i);
+
if ((i == cpu) || (has_mc && match_mc(c, o))) {
link_mask(core, cpu, i);

2012-06-01 06:48:23

by Kamalesh Babulal

[permalink] [raw]
Subject: Re: [PATCH] sched/x86: Calculate booted cores after construction of sibling_mask

* Peter Zijlstra <[email protected]> [2012-05-31 22:30:57]:

(snip)
> >
> > sched/x86: Calculate booted cores after construction of sibling_mask
>
> Why repeat this subject again?
>
Will take care of it from next posting.

(snip)
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index fd019d7..a8d78f3 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -382,7 +382,12 @@ void __cpuinit set_cpu_sibling_map(int cpu)
> > if ((i == cpu) || (has_mc && match_llc(c, o)))
> > link_mask(llc_shared, cpu, i);
> >
> > - if ((i == cpu) || (has_mc && match_mc(c, o))) {
> > + }
> > +
> > + for_each_cpu(i, cpu_sibling_setup_mask) {
> > + o = &cpu_data(i);
> > +
> > + if ((i == cpu) || (has_mc && match_mc(c, o))) {
> > link_mask(core, cpu, i);
> >
> > /*
>
> This patch actually horribly mangles the indenting. I did the below to
> it:

Sorry about the indentation. Thanks for fixing it.

2012-06-01 10:09:03

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] sched/x86: Calculate booted cores after construction of sibling_mask

On Fri, 2012-06-01 at 12:18 +0530, Kamalesh Babulal wrote:
> Will take care of it from next posting.

No need, I queued the fixup...

2012-06-06 15:53:06

by Kamalesh Babulal

[permalink] [raw]
Subject: [tip:sched/urgent] sched/x86: Calculate booted cores after construction of sibling_mask

Commit-ID: ceb1cbac8eda66cf0f889def226b4e82f8ff857b
Gitweb: http://git.kernel.org/tip/ceb1cbac8eda66cf0f889def226b4e82f8ff857b
Author: Kamalesh Babulal <[email protected]>
AuthorDate: Thu, 31 May 2012 13:07:38 +0530
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 6 Jun 2012 16:37:59 +0200

sched/x86: Calculate booted cores after construction of sibling_mask

Commit 316ad248307fb ("sched/x86: Rewrite set_cpu_sibling_map()")
broke the booted_cores accounting.

The problem is that the booted_cores accounting needs all the
sibling links set up. So restore the second loop and add a comment as
to why its needed.

On qemu booted with -smp sockets=1,cores=2,threads=2;
Before:
$ grep cores /proc/cpuinfo
cpu cores : 2
cpu cores : 1
cpu cores : 4
cpu cores : 3

With the patch:
$ grep cores /proc/cpuinfo
cpu cores : 2
cpu cores : 2
cpu cores : 2
cpu cores : 2

Reported-by: Prarit Bhargava <[email protected]>
Reported-by: Borislav Petkov <[email protected]>
Signed-off-by: Kamalesh Babulal <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/smpboot.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fd019d7..3fab55b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -382,6 +382,15 @@ void __cpuinit set_cpu_sibling_map(int cpu)
if ((i == cpu) || (has_mc && match_llc(c, o)))
link_mask(llc_shared, cpu, i);

+ }
+
+ /*
+ * This needs a separate iteration over the cpus because we rely on all
+ * cpu_sibling_mask links to be set-up.
+ */
+ for_each_cpu(i, cpu_sibling_setup_mask) {
+ o = &cpu_data(i);
+
if ((i == cpu) || (has_mc && match_mc(c, o))) {
link_mask(core, cpu, i);