Skip to contentSkip to conversation

x86_32: remove warning message not used

PATCH Applied Hiroshi Shimamoto 4 participants 15 messages Jan 14, 2008 – Jan 19, 2008 x86

lore
Share & tools
lore

Hiroshi Shimamoto

author
#0 lore raw

PATCH 1/1 x86_32: remove warning message not used

From: Hiroshi Shimamoto <[email protected]>

smp_num_siblings hasn't been updated at this point yet, so it's always 1. This polling and HT warning message is never shown.

Signed-off-by Hiroshi Shimamoto <[email protected]>
---
 arch/x86/kernel/process_32.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
arch/x86/kernel/process_32.c +0 -4
index 69a69c3..f449b6d 100644
@@ -305,10 +305,6 @@ static int __init idle_setup(char *str)
 	if (!strcmp(str, "poll")) {
 		printk("using polling idle threads.\n");
 		pm_idle = poll_idle;
-#ifdef CONFIG_X86_SMP
-		if (smp_num_siblings > 1)
-			printk("WARNING: polling idle and HT enabled, performance may degrade.\n");
-#endif
 	} else if (!strcmp(str, "mwait"))
 		force_mwait = 1;
 	else
Signature · 1 line
1.5.3.6
#1 lore raw
Quoted 4 lines · Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>

smp_num_siblings hasn't been updated at this point yet, so it's always
1. This polling and HT warning message is never shown.

hah, nice one. But could you perhaps move it to a place where it has a chance to be printed? The warning still makes sense.

Ingo

author
#2 lore raw
Quoted 10 lines · Ingo Molnar
Quoted 4 lines · Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>

smp_num_siblings hasn't been updated at this point yet, so it's always
1. This polling and HT warning message is never shown.

hah, nice one. But could you perhaps move it to a place where it has a chance to be printed? The warning still makes sense.

yeah, you're right. I'll do.

Thanks,
Hiroshi Shimamoto

author
#3 lore raw

PATCH 1/1 x86: move warning message of polling idle and HT enabled

In reply to Ingo Molnar

From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86: move warning message of polling idle and HT enabled

This warning at idle_setup() is never shown because smp_num_sibling hasn't been updated at this point yet.

Move this polling idle and HT enabled warning message to select_idle_routine(). I also implement this warning on 64-bit kernel and make select_idle_routine() call after detect_ht() call.

Signed-off-by Hiroshi Shimamoto <[email protected]>

---
Ingo, this patch needs Andi's patch;
i386: Move MWAIT idle check to generic CPU
http://lkml.org/lkml/2008/1/2/358
I haven't found it in x86.git tree.

 arch/x86/kernel/process_32.c |   18 ++++++++++++------
 arch/x86/kernel/process_64.c |   17 ++++++++++++-----
 arch/x86/kernel/setup_64.c   |    3 ++-
 3 files changed, 26 insertions(+), 12 deletions(-)
arch/x86/kernel/process_32.c +12 -6
index 69a69c3..d52c032 100644
@@ -287,17 +287,27 @@ static void mwait_idle(void)
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
+	static int selected;
+
+	if (selected)
+		return;
+#ifdef CONFIG_X86_SMP
+	if (pm_idle == poll_idle && smp_num_siblings > 1) {
+		printk(KERN_WARNING "WARNING: polling idle and HT enabled,"
+			" performance may degrade.\n");
+	}
+#endif
 	if (cpu_has(c, X86_FEATURE_MWAIT)) {
-		printk("monitor/mwait feature present.\n");
 		/*
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
 		if (!pm_idle) {
-			printk("using mwait in idle threads.\n");
+			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
 	}
+	selected = 1;
 }
 
 static int __init idle_setup(char *str)

@@ -305,10 +315,6 @@ static int __init idle_setup(char *str)
 	if (!strcmp(str, "poll")) {
 		printk("using polling idle threads.\n");
 		pm_idle = poll_idle;
-#ifdef CONFIG_X86_SMP
-		if (smp_num_siblings > 1)
-			printk("WARNING: polling idle and HT enabled, performance may degrade.\n");
-#endif
 	} else if (!strcmp(str, "mwait"))
 		force_mwait = 1;
 	else
arch/x86/kernel/process_64.c +12 -5
index 5e12edd..8cff606 100644
@@ -282,20 +282,27 @@ static void mwait_idle(void)
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
-	static int printed;
+	static int selected;
+
+	if (selected)
+		return;
+#ifdef CONFIG_X86_SMP
+	if (pm_idle == poll_idle && smp_num_siblings > 1) {
+		printk(KERN_WARNING "WARNING: polling idle and HT enabled,"
+			" performance may degrade.\n");
+	}
+#endif
 	if (cpu_has(c, X86_FEATURE_MWAIT)) {
 		/*
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
 		if (!pm_idle) {
-			if (!printed) {
-				printk(KERN_INFO "using mwait in idle threads.\n");
-				printed = 1;
-			}
+			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
 	}
+	selected = 1;
 }
 
 static int __init idle_setup(char *str)
arch/x86/kernel/setup_64.c +2 -1
index c8dcdd2..8ebf990 100644
@@ -1067,7 +1067,6 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 		break;
 	}
 
-	select_idle_routine(c);
 	detect_ht(c);
 
 	/*

@@ -1085,6 +1084,8 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_X86_MCE
 	mcheck_init(c);
 #endif
+	select_idle_routine(c);
+
 	if (c != &boot_cpu_data)
 		mtrr_ap_init();
 #ifdef CONFIG_NUMA
Signature · 1 line
1.5.3.6
#4 lore raw

Re: [PATCH] x86: move warning message of polling idle and HT enabled

Quoted 9 lines · Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86: move warning message of polling idle and HT enabled

This warning at idle_setup() is never shown because smp_num_sibling hasn't been updated at this point yet.

Move this polling idle and HT enabled warning message to select_idle_routine(). I also implement this warning on 64-bit kernel and make select_idle_routine() call after detect_ht() call.

looks good to me, but could you please split this up into two patches instead - one that just moves/adds the printks, the other one that moves the select_idle_routine() call? (Moving init calls around is notoriously error-prone, so we want as small of a bisection target as possible.) Thanks,

Ingo

author
#5 lore raw

Re: [PATCH] x86: move warning message of polling idle and HT enabled

Quoted 8 lines · Ingo Molnar
Quoted 6 lines · Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86: move warning message of polling idle and HT enabled

This warning at idle_setup() is never shown because smp_num_sibling hasn't been updated at this point yet.

Quoted 8 lines
Quoted 3 lines

Move this polling idle and HT enabled warning message to select_idle_routine(). I also implement this warning on 64-bit kernel and make select_idle_routine() call after detect_ht() call.

looks good to me, but could you please split this up into two patches instead - one that just moves/adds the printks, the other one that moves the select_idle_routine() call? (Moving init calls around is notoriously error-prone, so we want as small of a bisection target as possible.)

Sure, I'll repost take3 soon.

Thanks,
Hiroshi Shimamoto

author
#6 lore raw

cover x86: move warning message of polling idle and HT enabled, take 3

In reply to Ingo Molnar

This patch set for printing properly the warning message; "polling idle and HT enabled, performance may degrade"

The warning message is never shown because smp_num_sibling hasn't been updated yet at idle_setup().
So I moved the warning to select_idle_routine() and made it called after detect_ht().

It also needs Andi's patch for 32-bit.
i386: Move MWAIT idle check to generic CPU
http://lkml.org/lkml/2008/1/2/358

[PATCH 1/2] x86: move warning message of polling idle and HT enabled [PATCH 2/2] x86_64: move select_idle_routine() call after detect_ht()

Thanks,
Hiroshi Shimamoto

author
#7 lore raw

PATCH 1/2 x86: move warning message of polling idle and HT enabled

From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86: move warning message of polling idle and HT enabled

The warning message at idle_setup() is never shown because smp_num_sibling hasn't been updated at this point yet.

Move this polling idle and HT enabled warning to select_idle_routine(). I also implement this warning on 64-bit kernel.

Signed-off-by Hiroshi Shimamoto <[email protected]>
---
 arch/x86/kernel/process_32.c |   18 ++++++++++++------
 arch/x86/kernel/process_64.c |   17 ++++++++++++-----
 2 files changed, 24 insertions(+), 11 deletions(-)
arch/x86/kernel/process_32.c +12 -6
index 69a69c3..d52c032 100644
@@ -287,17 +287,27 @@ static void mwait_idle(void)
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
+	static int selected;
+
+	if (selected)
+		return;
+#ifdef CONFIG_X86_SMP
+	if (pm_idle == poll_idle && smp_num_siblings > 1) {
+		printk(KERN_WARNING "WARNING: polling idle and HT enabled,"
+			" performance may degrade.\n");
+	}
+#endif
 	if (cpu_has(c, X86_FEATURE_MWAIT)) {
-		printk("monitor/mwait feature present.\n");
 		/*
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
 		if (!pm_idle) {
-			printk("using mwait in idle threads.\n");
+			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
 	}
+	selected = 1;
 }
 
 static int __init idle_setup(char *str)

@@ -305,10 +315,6 @@ static int __init idle_setup(char *str)
 	if (!strcmp(str, "poll")) {
 		printk("using polling idle threads.\n");
 		pm_idle = poll_idle;
-#ifdef CONFIG_X86_SMP
-		if (smp_num_siblings > 1)
-			printk("WARNING: polling idle and HT enabled, performance may degrade.\n");
-#endif
 	} else if (!strcmp(str, "mwait"))
 		force_mwait = 1;
 	else
arch/x86/kernel/process_64.c +12 -5
index 5e12edd..8cff606 100644
@@ -282,20 +282,27 @@ static void mwait_idle(void)
 
 void __cpuinit select_idle_routine(const struct cpuinfo_x86 *c)
 {
-	static int printed;
+	static int selected;
+
+	if (selected)
+		return;
+#ifdef CONFIG_X86_SMP
+	if (pm_idle == poll_idle && smp_num_siblings > 1) {
+		printk(KERN_WARNING "WARNING: polling idle and HT enabled,"
+			" performance may degrade.\n");
+	}
+#endif
 	if (cpu_has(c, X86_FEATURE_MWAIT)) {
 		/*
 		 * Skip, if setup has overridden idle.
 		 * One CPU supports mwait => All CPUs supports mwait
 		 */
 		if (!pm_idle) {
-			if (!printed) {
-				printk(KERN_INFO "using mwait in idle threads.\n");
-				printed = 1;
-			}
+			printk(KERN_INFO "using mwait in idle threads.\n");
 			pm_idle = mwait_idle;
 		}
 	}
+	selected = 1;
 }
 
 static int __init idle_setup(char *str)
Signature · 1 line
1.5.3.6
Applied
#8 lore raw

Re: [PATCH 1/2] x86: move warning message of polling idle and HT enabled

Quoted 5 lines · Hiroshi Shimamoto

The warning message at idle_setup() is never shown because smp_num_sibling hasn't been updated at this point yet.

Move this polling idle and HT enabled warning to select_idle_routine(). I also implement this warning on 64-bit kernel.

thanks, applied.

Ingo

author
#9 lore raw

PATCH 2/2 x86_64: move select_idle_routine() call after detect_ht()

In reply to Hiroshi Shimamoto

From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86_64: move select_idle_routine() call after detect_ht()

Move the select_idle_routine() call to after the detect_ht() call at identify_cpu() on 64-bit.
This change is for printing the polling idle and HT enabled warning message properly.

Signed-off-by Hiroshi Shimamoto <[email protected]>
---
 arch/x86/kernel/setup_64.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kernel/setup_64.c +2 -1
index c8dcdd2..8ebf990 100644
@@ -1067,7 +1067,6 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 		break;
 	}
 
-	select_idle_routine(c);
 	detect_ht(c);
 
 	/*

@@ -1085,6 +1084,8 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_X86_MCE
 	mcheck_init(c);
 #endif
+	select_idle_routine(c);
+
 	if (c != &boot_cpu_data)
 		mtrr_ap_init();
 #ifdef CONFIG_NUMA
Signature · 1 line
1.5.3.6
Applied
#10 lore raw

Re: [PATCH 2/2] x86_64: move select_idle_routine() call after detect_ht()

Quoted 3 lines · Hiroshi Shimamoto

Move the select_idle_routine() call to after the detect_ht() call at identify_cpu() on 64-bit. This change is for printing the polling idle and HT enabled warning message properly.

thanks, applied.

Ingo

author
#11 lore raw

x86: kdump failure

In reply to Ingo Molnar

Hi Ingo,

on recent x86.git kernel fails to kdump with following BUG.

SysRq : Trigger a crashdump

------------[ cut here ]------------
kernel BUG at include/linux/elfcore.h:105!

invalid opcode: 0000 [1] PREEMPT SMP

In crash_save_cpu(), elf_core_copy_regs() is called and ELF_CORE_COPY_REGS macro is required because struct pt_regs and elf_gregset_t are different.

---
From: Hiroshi Shimamoto <[email protected]>
Subject: [PATCH] x86: kdump needs ELF_CORE_COPY_REGS macro

kdump needs ELF_CORE_COPY_REGS in crash_save_cpu(). This lack of the macro causes the following BUG.

SysRq : Trigger a crashdump

------------[ cut here ]------------
kernel BUG at include/linux/elfcore.h:105!

invalid opcode: 0000 [1] PREEMPT SMP

Signed-off-by Hiroshi Shimamoto <[email protected]>
---
 include/asm-x86/elf.h |   62 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)
include/asm-x86/elf.h +62 -0
index dab4744..d9c94e7 100644
@@ -106,6 +106,31 @@ extern unsigned int vdso_enabled;
 	_r->ax = 0; \
 } while (0)
 
+/*
+ * regs is struct pt_regs, pr_reg is elf_gregset_t (which is
+ * now struct_user_regs, they are different)
+ */
+
+#define ELF_CORE_COPY_REGS(pr_reg, regs) do {		\
+	pr_reg[0] = regs->bx;				\
+	pr_reg[1] = regs->cx;				\
+	pr_reg[2] = regs->dx;				\
+	pr_reg[3] = regs->si;				\
+	pr_reg[4] = regs->di;				\
+	pr_reg[5] = regs->bp;				\
+	pr_reg[6] = regs->ax;				\
+	pr_reg[7] = regs->ds & 0xffff;			\
+	pr_reg[8] = regs->es & 0xffff;			\
+	pr_reg[9] = regs->fs & 0xffff;			\
+	savesegment(gs, pr_reg[10]);			\
+	pr_reg[11] = regs->orig_ax;			\
+	pr_reg[12] = regs->ip;				\
+	pr_reg[13] = regs->cs & 0xffff;			\
+	pr_reg[14] = regs->flags;			\
+	pr_reg[15] = regs->sp;				\
+	pr_reg[16] = regs->ss & 0xffff;			\
+} while (0);
+
 #define ELF_PLATFORM	(utsname()->machine)
 #define set_personality_64bit()	do { } while (0)
 

@@ -165,6 +190,43 @@ static inline void elf_common_init(struct thread_struct *t,
 	} while (0)
 #define COMPAT_ELF_PLATFORM			("i686")
 
+/*
+ * regs is struct pt_regs, pr_reg is elf_gregset_t (which is
+ * now struct_user_regs, they are different). Assumes current is the process
+ * getting dumped.
+ */
+
+#define ELF_CORE_COPY_REGS(pr_reg, regs)  do {			\
+	unsigned v;						\
+	(pr_reg)[0] = (regs)->r15;				\
+	(pr_reg)[1] = (regs)->r14;				\
+	(pr_reg)[2] = (regs)->r13;				\
+	(pr_reg)[3] = (regs)->r12;				\
+	(pr_reg)[4] = (regs)->bp;				\
+	(pr_reg)[5] = (regs)->bx;				\
+	(pr_reg)[6] = (regs)->r11;				\
+	(pr_reg)[7] = (regs)->r10;				\
+	(pr_reg)[8] = (regs)->r9;				\
+	(pr_reg)[9] = (regs)->r8;				\
+	(pr_reg)[10] = (regs)->ax;				\
+	(pr_reg)[11] = (regs)->cx;				\
+	(pr_reg)[12] = (regs)->dx;				\
+	(pr_reg)[13] = (regs)->si;				\
+	(pr_reg)[14] = (regs)->di;				\
+	(pr_reg)[15] = (regs)->orig_ax;				\
+	(pr_reg)[16] = (regs)->ip;				\
+	(pr_reg)[17] = (regs)->cs;				\
+	(pr_reg)[18] = (regs)->flags;				\
+	(pr_reg)[19] = (regs)->sp;				\
+	(pr_reg)[20] = (regs)->ss;				\
+	(pr_reg)[21] = current->thread.fs;			\
+	(pr_reg)[22] = current->thread.gs;			\
+	asm("movl %%ds,%0" : "=r" (v)); (pr_reg)[23] = v;	\
+	asm("movl %%es,%0" : "=r" (v)); (pr_reg)[24] = v;	\
+	asm("movl %%fs,%0" : "=r" (v)); (pr_reg)[25] = v;	\
+	asm("movl %%gs,%0" : "=r" (v)); (pr_reg)[26] = v;	\
+} while (0);
+
 /* I'm not sure if we can use '-' here */
 #define ELF_PLATFORM       ("x86_64")
 extern void set_personality_64bit(void);
Signature · 1 line
1.5.3.7
Applied
#12 lore raw

Re: x86: kdump failure

Quoted 7 lines · Hiroshi Shimamoto

kdump needs ELF_CORE_COPY_REGS in crash_save_cpu(). This lack of the macro causes the following BUG.

SysRq : Trigger a crashdump

------------[ cut here ]------------
kernel BUG at include/linux/elfcore.h:105!

invalid opcode: 0000 [1] PREEMPT SMP

thanks, applied.

include/asm-x86/elf.h extern unsigned int vdso_enabled; · line 114

Quoted 6 lines
include/asm-x86/elf.h +6 -0
+/*
+ * regs is struct pt_regs, pr_reg is elf_gregset_t (which is
+ * now struct_user_regs, they are different)
+ */
+
+#define ELF_CORE_COPY_REGS(pr_reg, regs) do {		\

this macro got removed by the regset patches. Roland, any ideas?

Ingo

#13 lore raw

Re: x86: kdump failure

Quoted 21 lines · Ingo Molnar
Quoted 7 lines · Hiroshi Shimamoto

kdump needs ELF_CORE_COPY_REGS in crash_save_cpu(). This lack of the macro causes the following BUG.

SysRq : Trigger a crashdump

------------[ cut here ]------------
kernel BUG at include/linux/elfcore.h:105!

invalid opcode: 0000 [1] PREEMPT SMP

thanks, applied.

include/asm-x86/elf.h extern unsigned int vdso_enabled; · line 114

Quoted 6 lines
include/asm-x86/elf.h +6 -0
+/*
+ * regs is struct pt_regs, pr_reg is elf_gregset_t (which is
+ * now struct_user_regs, they are different)
+ */
+
+#define ELF_CORE_COPY_REGS(pr_reg, regs) do {		\

this macro got removed by the regset patches. Roland, any ideas?

Quoted 1 line

Ingo

Hi Ingo/Hiroshi Shimamoto,

There also has been a huge update on ltp-kdump test suite. You can find the same @ http://ltp.cvs.sourceforge.net/ltp/ltp/testcases/kdump/,

--Subrata

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#14 lore raw

Re: x86: kdump failure

In reply to Ingo Molnar

Oops, I overlooked the use of elf_core_copy_regs in kernel/kexec.c. It is certainly safe and fine to reintroduce the old macro. Everything removed in the "x86 user_regset cleanup" patch is purely removing code and it doesn't hurt to have it back (it's just all unused except for this kexec nit).

Unfortunately it really doesn't fit to have kexec call into the new user_regset code that replaced this macro for user core dump purposes. Those new interfaces are really purely for user-mode state, derived only from task_struct (i.e. uses task_pt_regs), not from a struct pt_regs pointer passed in. (There is the minority case where it really is using user-mode state. That part could be done via the user_regset interface, if that saved any trouble.)

Things like crash_fixup_ss_esp point to the poor fit of the code intended for user core dumps with what kexec needs. IMHO it would be cleaner for kexec's arch interfaces to fill in elf_gregset_t directly, replacing some of the places a struct pt_regs is passed around now. crash_setup_regs already has to know the name of every register anyway. A particular arch's definition can share code with its core dump or user_regset code when that fits.

Thanks,
Roland