[PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

__die() invokes later __show_regs() -> show_regs_print_info() which
prints the current preemption model.
Remove it from the initial line.

Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/kernel/traps.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 6ea645939573f..afbd2ebe5c39d 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -258,13 +258,6 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 	barrier();
 }
 
-#ifdef CONFIG_PREEMPT
-#define S_PREEMPT " PREEMPT"
-#elif defined(CONFIG_PREEMPT_RT)
-#define S_PREEMPT " PREEMPT_RT"
-#else
-#define S_PREEMPT ""
-#endif
 #ifdef CONFIG_SMP
 #define S_SMP " SMP"
 #else
@@ -282,8 +275,8 @@ static int __die(const char *str, int err, struct pt_regs *regs)
 	static int die_counter;
 	int ret;
 
-	pr_emerg("Internal error: %s: %x [#%d]" S_PREEMPT S_SMP S_ISA "\n",
-	         str, err, ++die_counter);
+	pr_emerg("Internal error: %s: %x [#%d]" S_SMP S_ISA "\n",
+		 str, err, ++die_counter);
 
 	/* trap and error numbers are mostly meaningless on ARM */
 	ret = notify_die(DIE_OOPS, str, regs, err, tsk->thread.trap_no, SIGSEGV);
-- 
2.47.2

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

On 2025-02-03 15:16:26 [+0100], To linux-kernel@vger.kernel.org wrote:
> __die() invokes later __show_regs() -> show_regs_print_info() which
> prints the current preemption model.
> Remove it from the initial line.
> 
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Is it okay, to route this via the sched tree?

Sebastian

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Russell King (Oracle)]

On Mon, Feb 10, 2025 at 01:04:29PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-02-03 15:16:26 [+0100], To linux-kernel@vger.kernel.org wrote:
> > __die() invokes later __show_regs() -> show_regs_print_info() which
> > prints the current preemption model.
> > Remove it from the initial line.
> > 
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Is it okay, to route this via the sched tree?

Sorry, its not obvious where show_regs_print_info() prints this.
dump_stack_print_info() itself doesn't directly. print_worker_info()
doesn't. print_stop_info() doesn't. Not sure whether print_scx_info()
does.

Maybe showing example output would help?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

On 2025-02-10 15:16:11 [+0000], Russell King (Oracle) wrote:
> On Mon, Feb 10, 2025 at 01:04:29PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2025-02-03 15:16:26 [+0100], To linux-kernel@vger.kernel.org wrote:
> > > __die() invokes later __show_regs() -> show_regs_print_info() which
> > > prints the current preemption model.
> > > Remove it from the initial line.
> > > 
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > 
> > Is it okay, to route this via the sched tree?
> 
> Sorry, its not obvious where show_regs_print_info() prints this.
> dump_stack_print_info() itself doesn't directly. print_worker_info()
> doesn't. print_stop_info() doesn't. Not sure whether print_scx_info()
> does.
> 
> Maybe showing example output would help?

Patch 2/9 adds this. [ https://lore.kernel.org/all/20250203141632.440554-3-bigeasy@linutronix.de/ ]

This this applied, die("test") on ARM ends as:

[    1.595106] Kernel panic - not syncing: test
[    1.596044] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.14.0-rc2-00009-gb80a798df08c-dirty #13 PREEMPT 
[    1.596768] Tainted: [W]=WARN
[    1.596946] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
[    1.597407] Call trace: 
[    1.598183]  unwind_backtrace from show_stack+0x10/0x14
[    1.599231]  show_stack from dump_stack_lvl+0x50/0x64
[    1.599492]  dump_stack_lvl from panic+0x114/0x358
[    1.599824]  panic from kernel_init+0x64/0x74
[    1.600048]  kernel_init from ret_from_fork+0x14/0x38
[    1.600349] Exception stack(0xdf829fb0 to 0xdf829ff8)
[    1.600730] 9fa0:                                     00000000 00000000 00000000 00000000
[    1.601102] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.601447] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    1.603163] ---[ end Kernel panic - not syncing: test ]---

Sebastian

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Russell King (Oracle)]

On Mon, Feb 10, 2025 at 04:39:02PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-02-10 15:16:11 [+0000], Russell King (Oracle) wrote:
> > On Mon, Feb 10, 2025 at 01:04:29PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2025-02-03 15:16:26 [+0100], To linux-kernel@vger.kernel.org wrote:
> > > > __die() invokes later __show_regs() -> show_regs_print_info() which
> > > > prints the current preemption model.
> > > > Remove it from the initial line.
> > > > 
> > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > 
> > > Is it okay, to route this via the sched tree?
> > 
> > Sorry, its not obvious where show_regs_print_info() prints this.
> > dump_stack_print_info() itself doesn't directly. print_worker_info()
> > doesn't. print_stop_info() doesn't. Not sure whether print_scx_info()
> > does.
> > 
> > Maybe showing example output would help?
> 
> Patch 2/9 adds this. [ https://lore.kernel.org/all/20250203141632.440554-3-bigeasy@linutronix.de/ ]

That explains it - patch 2 is only sent to a very restricted subset
and not even to mailing lists that would be relevant in the absence
of a direct Cc. Ditto patch 1. All I received were patches 3 and 4.

In patch 1:

+	static char buf[128];
...
+		seq_buf_init(&s, buf, 128);

Why not sizeof(buf) ?

For patch 2:

"Use
pr_warn() instead of printk() to pass a loglevel. This makes it part of
generic WARN/ BUG traces.
"

How about cases which use dump_stack_lvl() to dump the output at a more
severe level than warning? Should the message be printed at a less
severe level than the other messages?

> This this applied, die("test") on ARM ends as:
> 
> [    1.595106] Kernel panic - not syncing: test
> [    1.596044] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W 6.14.0-rc2-00009-gb80a798df08c-dirty #13 PREEMPT
> [    1.596768] Tainted: [W]=WARN
> [    1.596946] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022

Hmm. I've no idea what you're testing, what you've quoted makes zero
sense to me.

First...

void die(const char *str, struct pt_regs *regs, int err)

is the die function prototype, so it takes a bit more than what you've
indicated.

Second, "Kernel panic" suggests that panic() has been called. However,
this only happens when die() is called (or more specifically
oops_end()) from either interrupt context (in which case we get
"Kernel panic - Fatal exception in interrupt") or if panic_on_oops
is set ("Kernel panic - Fatal exception").

I don't see a path which would result in
"Kernel panic - not syncing: test" to be printed from this path.

Since __die() does not call dump_stack(), we're not going to call
dump_stack_print_info() from __die(), so I don't think it's appropriate
to remove this information.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

On 2025-02-10 16:26:56 [+0000], Russell King (Oracle) wrote:
> > Patch 2/9 adds this. [ https://lore.kernel.org/all/20250203141632.440554-3-bigeasy@linutronix.de/ ]
> 
> That explains it - patch 2 is only sent to a very restricted subset
> and not even to mailing lists that would be relevant in the absence
> of a direct Cc. Ditto patch 1. All I received were patches 3 and 4.
> 
> In patch 1:
> 
> +	static char buf[128];
> ...
> +		seq_buf_init(&s, buf, 128);
> 
> Why not sizeof(buf) ?

Indeed, why not. Updated.

> For patch 2:
> 
> "Use
> pr_warn() instead of printk() to pass a loglevel. This makes it part of
> generic WARN/ BUG traces.
> "

> How about cases which use dump_stack_lvl() to dump the output at a more
> severe level than warning? Should the message be printed at a less
> severe level than the other messages?

This is bad indeed. I reverted that piece.

> > This this applied, die("test") on ARM ends as:
> > 
> > [    1.595106] Kernel panic - not syncing: test
> > [    1.596044] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W 6.14.0-rc2-00009-gb80a798df08c-dirty #13 PREEMPT
> > [    1.596768] Tainted: [W]=WARN
> > [    1.596946] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
> 
> Hmm. I've no idea what you're testing, what you've quoted makes zero
> sense to me.
> 
> First...
> 
> void die(const char *str, struct pt_regs *regs, int err)
> 
> is the die function prototype, so it takes a bit more than what you've
> indicated.
> 
> Second, "Kernel panic" suggests that panic() has been called. However,
> this only happens when die() is called (or more specifically
> oops_end()) from either interrupt context (in which case we get
> "Kernel panic - Fatal exception in interrupt") or if panic_on_oops
> is set ("Kernel panic - Fatal exception").
> 
> I don't see a path which would result in
> "Kernel panic - not syncing: test" to be printed from this path.
> 
> Since __die() does not call dump_stack(), we're not going to call
> dump_stack_print_info() from __die(), so I don't think it's appropriate
> to remove this information.

Okay. Let me try again with a stack overflow during boot. With the
series:

| Freeing unused kernel image (initmem) memory: 2048K
| 8<--- cut here ---
| Unable to handle kernel paging request at virtual address df82a000 when write
| [df82a000] *pgd=80000040007003, *pmd=41487003, *pte=00000000
| Internal error: Oops: a07 [#1] SMP ARM
| Modules linked in:
| CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.14.0-rc2-00009-gdca8d546a8a3-dirty #16 PREEMPT
| Tainted: [W]=WARN
| Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
| PC is at mmioset+0x38/0xac
| LR is at 0x34343434
| pc : [<c0a07a58>]    lr : [<34343434>]    psr: 20000013
| sp : df829c88  ip : df829ff4  fp : 00000000
| r10: 00000000  r9 : 00000000  r8 : 34343434
| r7 : 00000000  r6 : 00000000  r5 : c0a33278  r4 : c1207bd4
| r3 : 34343434  r2 : 00000080  r1 : 34343434  r0 : df829ea4
| Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
| Control: 30c5387d  Table: 40003000  DAC: 00000001
| Register r0 information: 2-page vmalloc region starting at 0xdf828000 allocated at kernel_clone+0x58/0x3b8
…
| Process swapper/0 (pid: 1, stack limit = 0x4c737b1e)
| Stack: (0xdf829c88 to 0xdf82a000)
…
| Call trace:
|  mmioset from func+0x24/0x50
|  func from kernel_init+0x7c/0x168
|  kernel_init from 0x34343434
| Code: e1a08001 e1a0e003 e2522040 a8ac410a (a8ac410a)
| ---[ end trace 0000000000000000 ]---
| Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
| ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

Without:

| Freeing unused kernel image (initmem) memory: 2048K
| 8<--- cut here ---
| Unable to handle kernel paging request at virtual address df82a000 when write
| [df82a000] *pgd=80000040007003, *pmd=41487003, *pte=00000000
| Internal error: Oops: a07 [#1] PREEMPT SMP ARM
| Modules linked in:
| CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.14.0-rc2-00010-ged8f7288c355-dirty #17
| Tainted: [W]=WARN
| Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
| PC is at mmioset+0x38/0xac
| LR is at 0x34343434
| pc : [<c0a079d8>]    lr : [<34343434>]    psr: 20000113
| sp : df829c88  ip : df829ff4  fp : 00000000
| r10: 00000000  r9 : 00000000  r8 : 34343434
| r7 : 00000000  r6 : 00000000  r5 : c0a331ec  r4 : c1207bd4
| r3 : 34343434  r2 : 00000080  r1 : 34343434  r0 : df829ea4
| Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
| Control: 30c5387d  Table: 40003000  DAC: 00000001
| Register r0 information: 2-page vmalloc region starting at 0xdf828000 allocated at kernel_clone+0x58/0x3b8
…
| Process swapper/0 (pid: 1, stack limit = 0x7d941701)
| Stack: (0xdf829c88 to 0xdf82a000)
…
| Call trace:
|  mmioset from func+0x24/0x50
|  func from kernel_init+0x7c/0x168
|  kernel_init from 0x34343434
| Code: e1a08001 e1a0e003 e2522040 a8ac410a (a8ac410a)
| ---[ end trace 0000000000000000 ]---
| Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
| ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

As you see, the PREEMPT string moved and is still existing.

Sebastian

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Russell King (Oracle)]

On Tue, Feb 11, 2025 at 03:52:49PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-02-10 16:26:56 [+0000], Russell King (Oracle) wrote:
> > > This this applied, die("test") on ARM ends as:
> > > 
> > > [    1.595106] Kernel panic - not syncing: test
> > > [    1.596044] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W 6.14.0-rc2-00009-gb80a798df08c-dirty #13 PREEMPT
> > > [    1.596768] Tainted: [W]=WARN
> > > [    1.596946] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
> > 
> > Hmm. I've no idea what you're testing, what you've quoted makes zero
> > sense to me.
> > 
> > First...
> > 
> > void die(const char *str, struct pt_regs *regs, int err)
> > 
> > is the die function prototype, so it takes a bit more than what you've
> > indicated.
> > 
> > Second, "Kernel panic" suggests that panic() has been called. However,
> > this only happens when die() is called (or more specifically
> > oops_end()) from either interrupt context (in which case we get
> > "Kernel panic - Fatal exception in interrupt") or if panic_on_oops
> > is set ("Kernel panic - Fatal exception").
> > 
> > I don't see a path which would result in
> > "Kernel panic - not syncing: test" to be printed from this path.
> > 
> > Since __die() does not call dump_stack(), we're not going to call
> > dump_stack_print_info() from __die(), so I don't think it's appropriate
> > to remove this information.
> 
> Okay. Let me try again with a stack overflow during boot. With the
> series:
> 
> | Freeing unused kernel image (initmem) memory: 2048K
> | 8<--- cut here ---
> | Unable to handle kernel paging request at virtual address df82a000 when write
> | [df82a000] *pgd=80000040007003, *pmd=41487003, *pte=00000000
> | Internal error: Oops: a07 [#1] SMP ARM
> | Modules linked in:
> | CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.14.0-rc2-00009-gdca8d546a8a3-dirty #16 PREEMPT

Got it. Printed from __die() -> __show_regs() -> show_regs_print_info()
-> dump_stack_print_info().

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/9] arm: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

On 2025-02-11 15:05:38 [+0000], Russell King (Oracle) wrote:
> 
> Got it. Printed from __die() -> __show_regs() -> show_regs_print_info()
> -> dump_stack_print_info().
> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thank you.

> Thanks!

Sebastian

[PATCH v2 4/9] arm64: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

__die() invokes later show_regs() -> show_regs_print_info() which prints
the current preemption model.
Remove it from the initial line.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm64/kernel/traps.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4e26bd356a482..529cff825531c 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -172,14 +172,6 @@ static void dump_kernel_instr(const char *lvl, struct pt_regs *regs)
 	printk("%sCode: %s\n", lvl, str);
 }
 
-#ifdef CONFIG_PREEMPT
-#define S_PREEMPT " PREEMPT"
-#elif defined(CONFIG_PREEMPT_RT)
-#define S_PREEMPT " PREEMPT_RT"
-#else
-#define S_PREEMPT ""
-#endif
-
 #define S_SMP " SMP"
 
 static int __die(const char *str, long err, struct pt_regs *regs)
@@ -187,7 +179,7 @@ static int __die(const char *str, long err, struct pt_regs *regs)
 	static int die_counter;
 	int ret;
 
-	pr_emerg("Internal error: %s: %016lx [#%d]" S_PREEMPT S_SMP "\n",
+	pr_emerg("Internal error: %s: %016lx [#%d] " S_SMP "\n",
 		 str, err, ++die_counter);
 
 	/* trap and error numbers are mostly meaningless on ARM */
-- 
2.47.2

Re: [PATCH v2 4/9] arm64: Rely on generic printing of preemption model.

[Sebastian Andrzej Siewior]

On 2025-02-03 15:16:27 [+0100], To linux-kernel@vger.kernel.org wrote:
> __die() invokes later show_regs() -> show_regs_print_info() which prints
> the current preemption model.
> Remove it from the initial line.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Is it okay, to route this via the sched tree?

Sebastian

Re: [PATCH v2 4/9] arm64: Rely on generic printing of preemption model.

[Will Deacon]

On Mon, Feb 10, 2025 at 01:05:22PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-02-03 15:16:27 [+0100], To linux-kernel@vger.kernel.org wrote:
> > __die() invokes later show_regs() -> show_regs_print_info() which prints
> > the current preemption model.
> > Remove it from the initial line.
> > 
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Is it okay, to route this via the sched tree?

Fine by me:

Acked-by: Will Deacon <will@kernel.org>

Will