[PATCH] x86_32: remove warning message not used

[PATCH] x86_32: remove warning message not used

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

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 <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/process_32.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 69a69c3..f449b6d 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -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
-- 
1.5.3.6

Re: [PATCH] x86_32: remove warning message not used

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 
> 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

Re: [PATCH] x86_32: remove warning message not used

[Hiroshi Shimamoto]

Ingo Molnar wrote:
> * Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:
> 
>> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>>
>> 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

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

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
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 <h-shimamoto@ct.jp.nec.com>
---
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(-)

diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 69a69c3..d52c032 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -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
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 5e12edd..8cff606 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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)
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index c8dcdd2..8ebf990 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -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
-- 
1.5.3.6

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

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 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

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

[Hiroshi Shimamoto]

Ingo Molnar wrote:
> * Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:
> 
>> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>> 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.) 

Sure, I'll repost take3 soon.

Thanks,
Hiroshi Shimamoto

[PATCH 0/2] x86: move warning message of polling idle and HT enabled, take 3

[Hiroshi Shimamoto]

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

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

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
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 <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/process_32.c |   18 ++++++++++++------
 arch/x86/kernel/process_64.c |   17 ++++++++++++-----
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 69a69c3..d52c032 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -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
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 5e12edd..8cff606 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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)
-- 
1.5.3.6

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

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> 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

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

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
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 <h-shimamoto@ct.jp.nec.com>
---
 arch/x86/kernel/setup_64.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index c8dcdd2..8ebf990 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -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
-- 
1.5.3.6

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

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

> 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

x86: kdump failure

[Hiroshi Shimamoto]

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 <h-shimamoto@ct.jp.nec.com>
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 <h-shimamoto@ct.jp.nec.com>
---
 include/asm-x86/elf.h |   62 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/include/asm-x86/elf.h b/include/asm-x86/elf.h
index dab4744..d9c94e7 100644
--- a/include/asm-x86/elf.h
+++ b/include/asm-x86/elf.h
@@ -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);
-- 
1.5.3.7

Re: x86: kdump failure

[Ingo Molnar]

* Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:

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

> +/*
> + * 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

Re: x86: kdump failure

[Subrata Modak]

On Fri, 2008-01-18 at 10:02 +0100, Ingo Molnar wrote:
> * Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> wrote:
> 
> > 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.
> 
> > +/*
> > + * 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
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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: x86: kdump failure

[Roland McGrath]

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