Re: [RFC] Changing SysRq - show registers handling

Re: [RFC] Changing SysRq - show registers handling

[Andy Lutomirski]

Dmitry Torokhov wrote:

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

What about checking the flag on return from the input interrupts?  That way 
the overhead would be confined to code paths take the hit from passing an 
extra parameter.


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

--Andy

Re: [RFC] Changing SysRq - show registers handling

[Dmitry Torokhov]

On Thursday 03 June 2004 02:03 am, Andy Lutomirski wrote:
> 
> Dmitry Torokhov wrote:
> 
> > 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.
> 
> What about checking the flag on return from the input interrupts?  That way 
> the overhead would be confined to code paths take the hit from passing an 
> extra parameter.
>

It is hard to define what input interrupt is - PS/2 keyboard in KBD port (IRQ 1),
PS/2 keyboard in AUX port (IRQ 12), USB, serial port, parralel port keyboard
adapter... and all other achitectures taht have their means - it's impossible to
track them all.

-- 
Dmitry

[RFC] Changing SysRq - show registers handling

[Dmitry Torokhov]

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

Re: [RFC] Changing SysRq - show registers handling

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> 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.

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.

Re: [RFC] Changing SysRq - show registers handling

[Dmitry Torokhov]

On Thursday 03 June 2004 01:53 am, Andrew Morton wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> >
> > 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.
> 
> 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).

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.

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

-- 
Dmitry

Re: [RFC] Changing SysRq - show registers handling

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> > 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).
> 
>  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.

Re: [RFC] Changing SysRq - show registers handling

[Dmitry Torokhov]

On Thursday 03 June 2004 02:18 am, Andrew Morton wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> >
> > > 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).
> > 
> >  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 ;)

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

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

-- 
Dmitry

Re: [RFC] Changing SysRq - show registers handling

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> > 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 ;)

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

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

Re: [RFC] Changing SysRq - show registers handling

[Oliver Neukum]

Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:
> 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

Re: [RFC] Changing SysRq - show registers handling

[Theodore Ts'o]

On Thu, Jun 03, 2004 at 09:44:01AM +0200, Oliver Neukum wrote:
> Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:
> > 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

Re: [RFC] Changing SysRq - show registers handling

[Keith Owens]

On Thu, 3 Jun 2004 17:06:01 -0400, 
Theodore Ts'o <tytso@mit.edu> wrote:
>On Thu, Jun 03, 2004 at 09:44:01AM +0200, Oliver Neukum wrote:
>> Am Donnerstag, 3. Juni 2004 09:27 schrieb Dmitry Torokhov:
>> > 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.

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