Skip to contentSkip to conversation

Changing SysRq - show registers handling

RFC Dmitry Torokhov 5 participants 9 messages Jun 3, 2004

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

Hi,

Currently SysRq "show registers" command dumps registers and the call trace from keyboard interrupt context when SysRq-P. For that struct pt_regs * has to be dragged throughout entire input and USB systems. Other than passing this pointer to SysRq handler these systems has no interest in it, it is completely foreign piece of data for them and I would like to get rid of it.

I am suggesting slightly changing semantics of SysRq-P handling - instread of dumping registers and call trace immediately it will simply post a request for this information to be dumped. When next HW interrupt arrives and is handled, before running softirqs then current stack trace will be printed. This approach adds small overhead to the HW interrupt handling routine as the condition has to be checked with every interrupt but I expect it to be negligible as it is only check and conditional jump that is almost never taken. The code should be hot in cache so branch prediction should work just fine.

The patch below implements proposed changes in SysRq handler and adds necessary changes to do_IRQ() on i386. If it is agreed upon I will adjust other arches.

Please let me know what you think.

Signature · 92 lines
Dmitry

===== include/linux/sysrq.h 1.6 vs edited =====
--- 1.6/include/linux/sysrq.h	2004-05-15 01:11:58 -05:00
+++ edited/include/linux/sysrq.h	2004-06-01 00:29:09 -05:00
@@ -33,6 +33,17 @@
 void __handle_sysrq(int, struct pt_regs *, struct tty_struct *);
 
 /*
+ * Check whether register dump has been requested and print it
+ */
+extern unsigned int sysrq_register_dump_requested;
+void __sysrq_show_registers(struct pt_regs *);
+static inline void sysrq_show_registers(struct pt_regs *pt_regs)
+{
+	if (unlikely(sysrq_register_dump_requested != 0))
+		__sysrq_show_registers(pt_regs);
+}
+
+/*
  * Sysrq registration manipulation functions
  */
 
@@ -76,6 +87,10 @@
 }
 
 #else
+
+static inline void sysrq_show_registes(struct pt_regs *pt_regs)
+{
+}
 
 static inline int __reterr(void)
 {
===== drivers/char/sysrq.c 1.30 vs edited =====
--- 1.30/drivers/char/sysrq.c	2004-05-15 01:11:58 -05:00
+++ edited/drivers/char/sysrq.c	2004-06-01 00:29:08 -05:00
@@ -135,12 +135,33 @@
 
 
 /* SHOW SYSRQ HANDLERS BLOCK */
+unsigned int sysrq_register_dump_requested;
+static spinlock_t show_registers_lock = SPIN_LOCK_UNLOCKED;
+
+void __sysrq_show_registers(struct pt_regs *pt_regs)
+{
+	unsigned long flags;
+	int doit = 0;
+
+	spin_lock_irqsave(&show_registers_lock, flags);
+	if (sysrq_register_dump_requested) {
+		sysrq_register_dump_requested--;
+		doit = 1;
+	}
+	spin_unlock_irqrestore(&show_registers_lock, flags);
+
+	if (doit)
+		show_regs(pt_regs);
+}
 
 static void sysrq_handle_showregs(int key, struct pt_regs *pt_regs,
 				  struct tty_struct *tty) 
 {
-	if (pt_regs)
-		show_regs(pt_regs);
+	unsigned long flags;
+
+	spin_lock_irqsave(&show_registers_lock, flags);
+	sysrq_register_dump_requested++;
+	spin_unlock_irqrestore(&show_registers_lock, flags);
 }
 static struct sysrq_key_op sysrq_showregs_op = {
 	.handler	= sysrq_handle_showregs,
===== arch/i386/kernel/irq.c 1.53 vs edited =====
--- 1.53/arch/i386/kernel/irq.c	2004-05-29 02:26:35 -05:00
+++ edited/arch/i386/kernel/irq.c	2004-06-01 00:29:06 -05:00
@@ -34,6 +34,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/kallsyms.h>
+#include <linux/sysrq.h>
 
 #include <asm/atomic.h>
 #include <asm/io.h>
@@ -567,6 +568,8 @@
 	 */
 	desc->handler->end(irq);
 	spin_unlock(&desc->lock);
+
+	sysrq_show_registers(&regs);
 
 	irq_exit();
#1 lore raw
Quoted 7 lines · Dmitry Torokhov

Currently SysRq "show registers" command dumps registers and the call
trace from keyboard interrupt context when SysRq-P. For that struct pt_regs * has to be dragged throughout entire input and USB systems. Other than passing this pointer to SysRq handler these systems has no interest in it, it is completely foreign piece of data for them and I would like to get rid of it.

Quoted 9 lines

I am suggesting slightly changing semantics of SysRq-P handling - instread of dumping registers and call trace immediately it will simply post a request for this information to be dumped. When next HW interrupt arrives and is handled, before running softirqs then current stack trace will be printed. This approach adds small overhead to the HW interrupt handling routine as the condition has to be checked with every interrupt but I expect it to be negligible as it is only check and conditional jump that is almost never taken. The code should be hot in cache so branch prediction should work just fine.

Makes sense I guess.

There have been other times when I've needed access to the registers from within hard IRQ. But I forget the reason.

It would be more general, although a little slower to do:

DEFINE_PER_CPU(global_irq_regs);

do_IRQ(...)

{
	...
	struct pt_regs **cpu_regs_slot = __get_cpu_var(global_irq_regs);
	struct pt_regs *save = *cpu_regs_slot;
	*cpu_regs_slot = &regs;
	...
	*cpu_regs_slot = save;
}

And to teach the sysrq code to grab *__get_cpu_var(global_irq_regs).

Note that global_irq_regs is only valid if in_interrupt(). The sysrq handler can be called from process context via /proc/sysrq-trigger and should bale out if !in_interrupt().

+static inline void sysrq_show_registes(struct pt_regs *pt_regs)

typo.

author
#2 lore raw
Quoted 37 lines · Andrew Morton
Quoted 7 lines · Dmitry Torokhov

Currently SysRq "show registers" command dumps registers and the call
trace from keyboard interrupt context when SysRq-P. For that struct pt_regs * has to be dragged throughout entire input and USB systems. Other than passing this pointer to SysRq handler these systems has no interest in it, it is completely foreign piece of data for them and I would like to get rid of it.

Quoted 9 lines

I am suggesting slightly changing semantics of SysRq-P handling - instread of dumping registers and call trace immediately it will simply post a request for this information to be dumped. When next HW interrupt arrives and is handled, before running softirqs then current stack trace will be printed. This approach adds small overhead to the HW interrupt handling routine as the condition has to be checked with every interrupt but I expect it to be negligible as it is only check and conditional jump that is almost never taken. The code should be hot in cache so branch prediction should work just fine.

Makes sense I guess.

There have been other times when I've needed access to the registers from within hard IRQ. But I forget the reason.

It would be more general, although a little slower to do:

DEFINE_PER_CPU(global_irq_regs);

do_IRQ(...)

{
	...
	struct pt_regs **cpu_regs_slot = __get_cpu_var(global_irq_regs);
	struct pt_regs *save = *cpu_regs_slot;
	*cpu_regs_slot = &regs;
	...
	*cpu_regs_slot = save;
}
Quoted 1 line

And to teach the sysrq code to grab *__get_cpu_var(global_irq_regs).

Ok, so by making it a tad slower you can keep the old semantics - printing registers right when keyboard interrupt is processed instead of waiting till the next one arrives.

Hmm... the path is pretty hot, I am not sure what is best.

Quoted 9 lines

Note that global_irq_regs is only valid if in_interrupt(). The sysrq handler can be called from process context via /proc/sysrq-trigger and should bale out if !in_interrupt().

+static inline void sysrq_show_registes(struct pt_regs *pt_regs)

typo.

Yep, thanks for noticing.

Signature · 1 line
Dmitry
#3 lore raw
Quoted 11 lines · Dmitry Torokhov
Quoted 11 lines

do_IRQ(...)

{
	...
	struct pt_regs **cpu_regs_slot = __get_cpu_var(global_irq_regs);
	struct pt_regs *save = *cpu_regs_slot;
	*cpu_regs_slot = &regs;
	...
	*cpu_regs_slot = save;
}
Quoted 7 lines
Quoted 1 line

And to teach the sysrq code to grab *__get_cpu_var(global_irq_regs).

Ok, so by making it a tad slower you can keep the old semantics - printing registers right when keyboard interrupt is processed instead of waiting till the next one arrives.

Hmm... the path is pretty hot, I am not sure what is best.

Well bear in mind that we can then rip all the pt_regs passing out from everywhere, so as long as you edit every IRQ handler in the kernel it's a net win ;)

I _think_ it'll work - as long as all architectures go through a common dispatch function like do_IRQ(), which surely they do. The above code could be an arch-neutral inline actually.

I'm not sure what's best really. But something this general is more attractive than something which is purely for sysrs-T.

author
#4 lore raw
Quoted 18 lines · Andrew Morton
Quoted 17 lines · Dmitry Torokhov
Quoted 11 lines

do_IRQ(...)

{
	...
	struct pt_regs **cpu_regs_slot = __get_cpu_var(global_irq_regs);
	struct pt_regs *save = *cpu_regs_slot;
	*cpu_regs_slot = &regs;
	...
	*cpu_regs_slot = save;
}
Quoted 1 line

And to teach the sysrq code to grab *__get_cpu_var(global_irq_regs).

Ok, so by making it a tad slower you can keep the old semantics - printing registers right when keyboard interrupt is processed instead of waiting till the next one arrives.

Quoted 5 lines
Quoted 1 line

Hmm... the path is pretty hot, I am not sure what is best.

Well bear in mind that we can then rip all the pt_regs passing out from everywhere, so as long as you edit every IRQ handler in the kernel it's a net win ;)

Will you let me? :) USB and serio will at least not mess anyone else...

Quoted 8 lines

I _think_ it'll work - as long as all architectures go through a common dispatch function like do_IRQ(), which surely they do. The above code could be an arch-neutral inline actually.

I'm not sure what's best really. But something this general is more attractive than something which is purely for sysrs-T.

I don't like the requirement of SysRq request processing being in hard interrupt handler - that excludes uinput-generated events and precludes moving keyboard handling to a tasklet for example.

Signature · 1 line
Dmitry
#5 lore raw
Quoted 6 lines · Dmitry Torokhov
Quoted 4 lines

Well bear in mind that we can then rip all the pt_regs passing out from everywhere, so as long as you edit every IRQ handler in the kernel it's a net win ;)

Will you let me? :) USB and serio will at least not mess anyone else...

There are only 1300 of them. Could do it in three hours. Don't tempt me ;)

Quoted 5 lines
Quoted 5 lines

I _think_ it'll work - as long as all architectures go through a common dispatch function like do_IRQ(), which surely they do. The above code could be an arch-neutral inline actually.

Quoted 7 lines
Quoted 3 lines

I'm not sure what's best really. But something this general is more attractive than something which is purely for sysrs-T.

I don't like the requirement of SysRq request processing being in hard interrupt handler - that excludes uinput-generated events and precludes moving keyboard handling to a tasklet for example.

Ho hum. May as well run with your original idea I guess.

#6 lore raw
In reply to Dmitry Torokhov

Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:

Quoted 3 lines

I don't like the requirement of SysRq request processing being in hard interrupt handler - that excludes uinput-generated events and precludes moving keyboard handling to a tasklet for example.

SysRq should work even if bottom halfs don't.

Regards
Oliver

#7 lore raw
Quoted 6 lines · Oliver Neukum

Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:

Quoted 3 lines

I don't like the requirement of SysRq request processing being in hard interrupt handler - that excludes uinput-generated events and precludes moving keyboard handling to a tasklet for example.

SysRq should work even if bottom halfs don't.

Indeed; one of the times when SysRq-p is used in the field is when the machine is completely wedged. Sometimes it's the only way to figure out where the machine is wedged. It would be unfortunate if the number of cases (when the kernel is four feet in the air and twitching) where SysRq worked decreases as a result of the proposed change.

- Ted

#8 lore raw
Quoted 6 lines · Theodore Ts'o
Quoted 5 lines · Oliver Neukum

Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:

Quoted 3 lines

I don't like the requirement of SysRq request processing being in hard interrupt handler - that excludes uinput-generated events and precludes moving keyboard handling to a tasklet for example.

Quoted 8 lines
Quoted 1 line

SysRq should work even if bottom halfs don't.

Indeed; one of the times when SysRq-p is used in the field is when the machine is completely wedged. Sometimes it's the only way to figure out where the machine is wedged. It would be unfortunate if the number of cases (when the kernel is four feet in the air and twitching) where SysRq worked decreases as a result of the proposed change.

KDB has a similar problem, it needs struct pt_regs as a starting point for backtraces and that is not always available. I get around the lack of a pt_regs by using KDB_ENTER() which generates a software interrupt. We could define a software interrupt for SYSRQ_ENTER(key). This would remove all the overhead of saving and tracking pt_regs from the fast paths. The downside is that it needs a software interrupt to be defined for every arch :(.