x86: show cpuinfo only for online CPUs

PATCH Superseded Andreas Herrmann 3 participants 7 messages Nov 1, 2007 x86

Share & tools

Andreas Herrmann

author

2007-11-01 17:03 UTC #0 lore raw

PATCH 1/1 x86: show cpuinfo only for online CPUs

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

Regards,

Andreas

Signature · 31 lines

[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 <[email protected]>
---
 arch/x86/kernel/cpu/proc.c |   13 ++++---------
 arch/x86/kernel/setup_64.c |   15 ++++-----------
 2 files changed, 8 insertions(+), 20 deletions(-)

arch/x86/kernel/cpu/proc.c +4 -9

index 066f8c6..13cfd85 100644
@@ -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)

arch/x86/kernel/setup_64.c +4 ~~-11~~

index ec59fcb..480230a 100644
@@ -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);
 }

Signature · 1 line

1.5.3.4

Glauber de Oliveira Costa

2007-11-01 17:11 UTC #1 lore raw

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

Andreas Herrmann escreveu:

Quoted 8 lines

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-----

Andreas Herrmann3

author

2007-11-01 17:39 UTC #2 lore raw

Quoted 3 lines · Glauber de Oliveira Costa

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).

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Signature · 5 lines

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

Andreas Herrmann3

author

2007-11-01 18:40 UTC #3 lore raw

Quoted 6 lines · On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:

Quoted 3 lines · Glauber de Oliveira Costa
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

Signature · 5 lines

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

Glauber de Oliveira Costa

2007-11-01 18:44 UTC #4 lore raw

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

Andreas Herrmann3 escreveu:

Quoted 9 lines · On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:

Quoted 5 lines · Glauber de Oliveira Costa
Quoted 3 lines
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-----

H. Peter Anvin

reviewer

2007-11-01 19:13 UTC #5 lore raw

Quoted 9 lines · Glauber de Oliveira Costa

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Andreas Herrmann3 escreveu:
Quoted 5 lines · On Thu, Nov 01, 2007 at 06:35:43PM +0100, Andreas Herrmann3 wrote:
Quoted 4 lines · Glauber de Oliveira Costa
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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

Andreas Herrmann

author

2007-11-01 18:32 UTC #6 lore raw

PATCH v2 1/1 x86: show cpuinfo only for online CPUs

In reply to 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 <[email protected]>

---
 arch/x86/kernel/cpu/proc.c |    8 +++-----
 arch/x86/kernel/setup_64.c |    8 +++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

arch/x86/kernel/cpu/proc.c +3 -5

index 066f8c6..3900e46 100644
@@ -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)

arch/x86/kernel/setup_64.c +3 -5

index ec59fcb..30d94d1 100644
@@ -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);
 }

Signature · 1 line

1.5.3.4