[PATCH] x86: show cpuinfo only for online CPUs

[PATCH] x86: show cpuinfo only for online CPUs

[Andreas Herrmann]

This patch applies on current Linus' git with Glauber's recent cpuinfo fix
(http://marc.info/?l=linux-kernel&m=119392088227245) applied.


Regards,

Andreas

-- 
[PATCH] x86: show cpuinfo only for online CPUs

Fix regressions introduced with 92cb7612aee39642d109b8d935ad265e602c0563.

It can happen that cpuinfo is displayed for CPUs that are not online or
even worse for CPUs not present at all. As an example, following was
shown for a "second" CPU of a single core K8 variant:

    processor       : 0
    vendor_id       : unknown
    cpu family      : 0
    model           : 0
    model name      : unknown
    stepping        : 0
    cache size      : 0 KB
    fpu             : yes
    fpu_exception   : yes
    cpuid level     : 0
    wp              : yes
    flags           :
    bogomips        : 0.00
    clflush size    : 0
    cache_alignment : 0
    address sizes   : 0 bits physical, 0 bits virtual
    power management:

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/kernel/cpu/proc.c |   13 ++++---------
 arch/x86/kernel/setup_64.c |   15 ++++-----------
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 066f8c6..13cfd85 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -85,14 +85,9 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		/* nothing */
 	};
 	struct cpuinfo_x86 *c = v;
-	int i, n = 0;
+	int i, n = c->cpu_index;
 	int fpu_exception;
 
-#ifdef CONFIG_SMP
-	if (!cpu_online(n))
-		return 0;
-	n = c->cpu_index;
-#endif
 	seq_printf(m, "processor\t: %d\n"
 		"vendor_id\t: %s\n"
 		"cpu family\t: %d\n"
@@ -177,14 +172,14 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 static void *c_start(struct seq_file *m, loff_t *pos)
 {
 	if (*pos == 0)	/* just in case, cpu 0 is not the first */
-		*pos = first_cpu(cpu_possible_map);
-	if ((*pos) < NR_CPUS && cpu_possible(*pos))
+		*pos = first_cpu(cpu_online_map);
+	if ((*pos) < NR_CPUS && cpu_online(*pos))
 		return &cpu_data(*pos);
 	return NULL;
 }
 static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	*pos = next_cpu(*pos, cpu_possible_map);
+	*pos = next_cpu(*pos, cpu_online_map);
 	return c_start(m, pos);
 }
 static void c_stop(struct seq_file *m, void *v)
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index ec59fcb..480230a 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -998,7 +998,7 @@ void __cpuinit print_cpu_info(struct cpuinfo_x86 *c)
 static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	struct cpuinfo_x86 *c = v;
-	int cpu = 0;
+	int cpu = c->cpu_index;
 
 	/* 
 	 * These flag bits must match the definitions in <asm/cpufeature.h>.
@@ -1075,13 +1075,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		/* nothing */
 	};
 
-
-#ifdef CONFIG_SMP
-	if (!cpu_online(c->cpu_index))
-		return 0;
-	cpu = c->cpu_index;
-#endif
-
 	seq_printf(m,"processor\t: %u\n"
 		     "vendor_id\t: %s\n"
 		     "cpu family\t: %d\n"
@@ -1170,15 +1163,15 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 static void *c_start(struct seq_file *m, loff_t *pos)
 {
 	if (*pos == 0)	/* just in case, cpu 0 is not the first */
-		*pos = first_cpu(cpu_possible_map);
-	if ((*pos) < NR_CPUS && cpu_possible(*pos))
+		*pos = first_cpu(cpu_online_map);
+	if ((*pos) < NR_CPUS && cpu_online(*pos))
 		return &cpu_data(*pos);
 	return NULL;
 }
 
 static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	*pos = next_cpu(*pos, cpu_possible_map);
+	*pos = next_cpu(*pos, cpu_online_map);
 	return c_start(m, pos);
 }
 
-- 
1.5.3.4

Re: [PATCH] x86: show cpuinfo only for online CPUs

[Glauber de Oliveira Costa]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andreas Herrmann escreveu:
> This patch applies on current Linus' git with Glauber's recent cpuinfo fix
> (http://marc.info/?l=linux-kernel&m=119392088227245) applied.
> 
> 
> Regards,
> 
> Andreas
> 

Andreas,

did you test it in a kernel without SMP compiled in ?

I'm a little afraid about what can happen here:

- --- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -998,7 +998,7 @@ void __cpuinit print_cpu_info(struct cpuinfo_x86 *c)
 static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	struct cpuinfo_x86 *c = v;
- -	int cpu = 0;
+	int cpu = c->cpu_index;

The attribuition of cpu_index only happens in a code that seems to be
called for all cpus but the boot one.

So it could even work, but as accident. Unless I'm wrong about it, I'd
prefer to see an explicit attribution of cpu_index = 0 somewhere for the
boot cpu.

Other than that, good catch! I didn't notice it, because all my present
cpus were online in my box
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFHKgj5jYI8LaFUWXMRAsU1AJ4ixYO42zHGK3Kx5SXH+cdly/rZ6wCfX6Kh
q2OCPt2qTniPmMNz+MzXc60=
=mYoV
-----END PGP SIGNATURE-----

Re: [PATCH] x86: show cpuinfo only for online CPUs

[Andreas Herrmann3]

On Thu, Nov 01, 2007 at 03:12:25PM -0200, Glauber de Oliveira Costa wrote:
> did you test it in a kernel without SMP compiled in ?
> 
> I'm a little afraid about what can happen here:

No I didn't, but should have done so - cpu_index exists only
in the SMP case ...

I'll come up with a corrected fix (adding some ifdefs).

> The attribuition of cpu_index only happens in a code that seems to be
> called for all cpus but the boot one.

Yes, just in identify_cpu and not in its "early_" variant.

> So it could even work, but as accident. Unless I'm wrong about it, I'd
> prefer to see an explicit attribution of cpu_index = 0 somewhere for the
> boot cpu.

Hmm, will look at this as well.

> Other than that, good catch! I didn't notice it, because all my present
> cpus were online in my box

You might be able to trigger it by using additional_cpus=<n> in your kernel
command line.
A second test is offlining a CPU: Then cpuinfo was still shown for the
offlined CPU.


Regards,

Andreas

-- 
Operating | AMD Saxony Limited Liability Company & Co. KG,
  System  | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 Research | Register Court Dresden: HRA 4896, General Partner authorized
  Center  | to represent: AMD Saxony LLC (Wilmington, Delaware, US)
  (OSRC)  | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH] x86: show cpuinfo only for online CPUs

[Andreas Herrmann3]

On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:
> On Thu, Nov 01, 2007 at 03:12:25PM -0200, Glauber de Oliveira Costa wrote:
> > So it could even work, but as accident. Unless I'm wrong about it, I'd
> > prefer to see an explicit attribution of cpu_index = 0 somewhere for the
> > boot cpu.
> 
> Hmm, will look at this as well.

BTW, isn't it zero initialized anyway?
So, no need to explicitely set cpuinfo->cpu_index=0 for the boot cpu.


Regards,

Andreas

-- 
Operating | AMD Saxony Limited Liability Company & Co. KG,
  System  | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 Research | Register Court Dresden: HRA 4896, General Partner authorized
  Center  | to represent: AMD Saxony LLC (Wilmington, Delaware, US)
  (OSRC)  | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH] x86: show cpuinfo only for online CPUs

[Glauber de Oliveira Costa]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andreas Herrmann3 escreveu:
> On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:
>> On Thu, Nov 01, 2007 at 03:12:25PM -0200, Glauber de Oliveira Costa wrote:
>>> So it could even work, but as accident. Unless I'm wrong about it, I'd
>>> prefer to see an explicit attribution of cpu_index = 0 somewhere for the
>>> boot cpu.
>> Hmm, will look at this as well.
> 
> BTW, isn't it zero initialized anyway?
> So, no need to explicitely set cpuinfo->cpu_index=0 for the boot cpu.

Well, it should be, but as far as I know it is not exactly a guarantee
given by all compilers. So it should be safer to do it explicitly, with
no prejudice. Unless it's really guaranteed. If it is, yeah, no need.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFHKh6hjYI8LaFUWXMRAmVXAJ47DCzJjWIQQ/duiVXtevsw1Y5gaQCfSOoU
uXq3vfX/NQjWk1lGQJCW6+E=
=qhGb
-----END PGP SIGNATURE-----

Re: [PATCH] x86: show cpuinfo only for online CPUs

[H. Peter Anvin]

Glauber de Oliveira Costa wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Andreas Herrmann3 escreveu:
>> On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:
>>> On Thu, Nov 01, 2007 at 03:12:25PM -0200, Glauber de Oliveira Costa wrote:
>>>> So it could even work, but as accident. Unless I'm wrong about it, I'd
>>>> prefer to see an explicit attribution of cpu_index = 0 somewhere for the
>>>> boot cpu.
>>> Hmm, will look at this as well.
>> BTW, isn't it zero initialized anyway?
>> So, no need to explicitely set cpuinfo->cpu_index=0 for the boot cpu.
> 
> Well, it should be, but as far as I know it is not exactly a guarantee
> given by all compilers. So it should be safer to do it explicitly, with
> no prejudice. Unless it's really guaranteed. If it is, yeah, no need.

Any uninitialized field in a static section (.data or .bss) is 
guaranteed to be initialized to zero.  This is guaranteed by the C 
standard.  In the former case, it is the responsibility of the compiler 
and in the latter by the runtime.

The Linux percpu handling creates a .data.percpu section which is then 
replicated into each of the CPU data blocks; thus, percpu data counts as 
static data for this purpose, and initialization is guaranteed.

	-hpa

[PATCH v2] x86: show cpuinfo only for online CPUs

[Andreas Herrmann]

New version of the fix.
Now it works also in the non-SMP case.
Tested on x86_64.

Regards,

Andreas
--

[PATCH] x86: show cpuinfo only for online CPUs

Fix regressions introduced with 92cb7612aee39642d109b8d935ad265e602c0563.

It can happen that cpuinfo is displayed for CPUs that are not online or
even worse for CPUs not present at all. As an example, following was
shown for a "second" CPU of a single core K8 variant:

    processor       : 0
    vendor_id       : unknown
    cpu family      : 0
    model           : 0
    model name      : unknown
    stepping        : 0
    cache size      : 0 KB
    fpu             : yes
    fpu_exception   : yes
    cpuid level     : 0
    wp              : yes
    flags           :
    bogomips        : 0.00
    clflush size    : 0
    cache_alignment : 0
    address sizes   : 0 bits physical, 0 bits virtual
    power management:

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/kernel/cpu/proc.c |    8 +++-----
 arch/x86/kernel/setup_64.c |    8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 066f8c6..3900e46 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -89,8 +89,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 	int fpu_exception;
 
 #ifdef CONFIG_SMP
-	if (!cpu_online(n))
-		return 0;
 	n = c->cpu_index;
 #endif
 	seq_printf(m, "processor\t: %d\n"
@@ -177,14 +175,14 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 static void *c_start(struct seq_file *m, loff_t *pos)
 {
 	if (*pos == 0)	/* just in case, cpu 0 is not the first */
-		*pos = first_cpu(cpu_possible_map);
-	if ((*pos) < NR_CPUS && cpu_possible(*pos))
+		*pos = first_cpu(cpu_online_map);
+	if ((*pos) < NR_CPUS && cpu_online(*pos))
 		return &cpu_data(*pos);
 	return NULL;
 }
 static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	*pos = next_cpu(*pos, cpu_possible_map);
+	*pos = next_cpu(*pos, cpu_online_map);
 	return c_start(m, pos);
 }
 static void c_stop(struct seq_file *m, void *v)
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index ec59fcb..30d94d1 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -1077,8 +1077,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 
 
 #ifdef CONFIG_SMP
-	if (!cpu_online(c->cpu_index))
-		return 0;
 	cpu = c->cpu_index;
 #endif
 
@@ -1170,15 +1168,15 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 static void *c_start(struct seq_file *m, loff_t *pos)
 {
 	if (*pos == 0)	/* just in case, cpu 0 is not the first */
-		*pos = first_cpu(cpu_possible_map);
-	if ((*pos) < NR_CPUS && cpu_possible(*pos))
+		*pos = first_cpu(cpu_online_map);
+	if ((*pos) < NR_CPUS && cpu_online(*pos))
 		return &cpu_data(*pos);
 	return NULL;
 }
 
 static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	*pos = next_cpu(*pos, cpu_possible_map);
+	*pos = next_cpu(*pos, cpu_online_map);
 	return c_start(m, pos);
 }
 
-- 
1.5.3.4