public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] sysrq: add debug message to reboot event
@ 2009-08-17 19:16 tina.yang
  2009-08-18 11:40 ` Jiri Kosina
  0 siblings, 1 reply; 6+ messages in thread
From: tina.yang @ 2009-08-17 19:16 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jiri Kosina

Add debug message to detect keyboard vs non-keyboard
triggered sysrq-b events.

Signed-off-by: Tina Yang <Tina.Yang@oracle.com>
Signed-off-by: Zach Brown <Zach.Brown@oracle.com>

---

--- linux-2.6.git/drivers/char/sysrq.c.orig     2009-08-13
15:43:34.000000000 -0700
+++ linux-2.6.git/drivers/char/sysrq.c  2009-08-13 21:51:15.832694000 -0700
@@ -135,17 +135,22 @@ static struct sysrq_key_op sysrq_crash_o
        .enable_mask    = SYSRQ_ENABLE_DUMP,
 };

-static void sysrq_handle_reboot(int key, struct tty_struct *tty)
+static void sysrq_handle_reboot(int check_mask, int key, struct
tty_struct *tty)
 {
+       if (check_mask)
+               printk("<keyboard-invoked>\n");
+        else
+               printk("<non-keyboard-invoked: PID %d COMMAND %s>\n",
+                               current->pid, current->comm);
        lockdep_off();
        local_irq_enable();
        emergency_restart();
 }
 static struct sysrq_key_op sysrq_reboot_op = {
-       .handler        = sysrq_handle_reboot,
-       .help_msg       = "reBoot",
-       .action_msg     = "Resetting",
-       .enable_mask    = SYSRQ_ENABLE_BOOT,
+       .handler_with_mask      = sysrq_handle_reboot,
+       .help_msg               = "reBoot",
+       .action_msg             = "Resetting",
+       .enable_mask            = SYSRQ_ENABLE_BOOT,
 };

 static void sysrq_handle_sync(int key, struct tty_struct *tty)
@@ -509,7 +514,10 @@ void __handle_sysrq(int key, struct tty_
                if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
                        printk("%s\n", op_p->action_msg);
                        console_loglevel = orig_log_level;
-                       op_p->handler(key, tty);
+                       if (op_p->handler_with_mask)
+                               op_p->handler_with_mask(check_mask, key,
tty);
+                       else
+                               op_p->handler(key, tty);
                } else {
                        printk("This sysrq operation is disabled.\n");
                }
--- linux-2.6.git/include/linux/sysrq.h.orig    2009-08-13
20:53:48.085786000 -0700
+++ linux-2.6.git/include/linux/sysrq.h 2009-08-13 20:54:12.625756000 -0700
@@ -32,6 +32,7 @@ struct tty_struct;

 struct sysrq_key_op {
        void (*handler)(int, struct tty_struct *);
+       void (*handler_with_mask)(int, int, struct tty_struct *);
        char *help_msg;
        char *action_msg;
        int enable_mask;


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sysrq: add debug message to reboot event
  2009-08-17 19:16 [PATCH] sysrq: add debug message to reboot event tina.yang
@ 2009-08-18 11:40 ` Jiri Kosina
  2009-08-19 20:58   ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2009-08-18 11:40 UTC (permalink / raw)
  To: tina.yang; +Cc: linux-kernel, Andrew Morton

On Mon, 17 Aug 2009, tina.yang wrote:

> Add debug message to detect keyboard vs non-keyboard
> triggered sysrq-b events.
> 
> Signed-off-by: Tina Yang <Tina.Yang@oracle.com>
> Signed-off-by: Zach Brown <Zach.Brown@oracle.com>
> 
> ---
> 
> --- linux-2.6.git/drivers/char/sysrq.c.orig     2009-08-13
> 15:43:34.000000000 -0700
> +++ linux-2.6.git/drivers/char/sysrq.c  2009-08-13 21:51:15.832694000 -0700
> @@ -135,17 +135,22 @@ static struct sysrq_key_op sysrq_crash_o
>         .enable_mask    = SYSRQ_ENABLE_DUMP,
>  };
> 
> -static void sysrq_handle_reboot(int key, struct tty_struct *tty)
> +static void sysrq_handle_reboot(int check_mask, int key, struct
> tty_struct *tty)
>  {
> +       if (check_mask)
> +               printk("<keyboard-invoked>\n");
> +        else
> +               printk("<non-keyboard-invoked: PID %d COMMAND %s>\n",
> +                               current->pid, current->comm);
>         lockdep_off();
>         local_irq_enable();
>         emergency_restart();
>  }
>  static struct sysrq_key_op sysrq_reboot_op = {
> -       .handler        = sysrq_handle_reboot,
> -       .help_msg       = "reBoot",
> -       .action_msg     = "Resetting",
> -       .enable_mask    = SYSRQ_ENABLE_BOOT,
> +       .handler_with_mask      = sysrq_handle_reboot,
> +       .help_msg               = "reBoot",
> +       .action_msg             = "Resetting",
> +       .enable_mask            = SYSRQ_ENABLE_BOOT,
>  };
> 
>  static void sysrq_handle_sync(int key, struct tty_struct *tty)
> @@ -509,7 +514,10 @@ void __handle_sysrq(int key, struct tty_
>                 if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
>                         printk("%s\n", op_p->action_msg);
>                         console_loglevel = orig_log_level;
> -                       op_p->handler(key, tty);
> +                       if (op_p->handler_with_mask)
> +                               op_p->handler_with_mask(check_mask, key,
> tty);
> +                       else
> +                               op_p->handler(key, tty);
>                 } else {
>                         printk("This sysrq operation is disabled.\n");
>                 }
> --- linux-2.6.git/include/linux/sysrq.h.orig    2009-08-13
> 20:53:48.085786000 -0700
> +++ linux-2.6.git/include/linux/sysrq.h 2009-08-13 20:54:12.625756000 -0700
> @@ -32,6 +32,7 @@ struct tty_struct;
> 
>  struct sysrq_key_op {
>         void (*handler)(int, struct tty_struct *);
> +       void (*handler_with_mask)(int, int, struct tty_struct *);
>         char *help_msg;
>         char *action_msg;
>         int enable_mask;
> 

I'd rather have this go through -mm, this doesn't really fully qualify for 
trivial tree. CCing Andrew.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sysrq: add debug message to reboot event
  2009-08-18 11:40 ` Jiri Kosina
@ 2009-08-19 20:58   ` Andrew Morton
  2009-08-20  2:22     ` tina.yang
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-08-19 20:58 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: tina.yang, linux-kernel

On Tue, 18 Aug 2009 13:40:24 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:

> On Mon, 17 Aug 2009, tina.yang wrote:
> 
> > Add debug message to detect keyboard vs non-keyboard
> > triggered sysrq-b events.
> > 
> > Signed-off-by: Tina Yang <Tina.Yang@oracle.com>
> > Signed-off-by: Zach Brown <Zach.Brown@oracle.com>
> > 
>
> ...
>
> 
> I'd rather have this go through -mm, this doesn't really fully qualify for 
> trivial tree. CCing Andrew.

The changelog had no description of why the change should be applied
and I was unable to understand the motivation from reading the patch,
so I skipped it.

Also the patch was wordwrapped and had all its tabs replaced by spaces.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sysrq: add debug message to reboot event
  2009-08-19 20:58   ` Andrew Morton
@ 2009-08-20  2:22     ` tina.yang
  2009-08-20 22:10       ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: tina.yang @ 2009-08-20  2:22 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Jiri Kosina, linux-kernel

Add debug message to detect keyboard vs non-keyboard triggered sysrq-b events.
This is to assist postmortem debugging on complicated computing setup with
large number of applications involved where reboot event had occurred, but 
unclear of its origin.

Signed-off-by: Tina Yang <Tina.Yang@oracle.com>
Signed-off-by: Zach Brown <Zach.Brown@oracle.com>

---

--- linux-2.6.18.i686/drivers/char/sysrq.c.orig	2009-08-13 10:55:57.526459000 -0700
+++ linux-2.6.18.i686/drivers/char/sysrq.c	2009-08-13 10:58:10.798739000 -0700
@@ -112,18 +112,23 @@ static struct sysrq_key_op sysrq_crashdu
 	.enable_mask	= SYSRQ_ENABLE_DUMP,
 };
 
-static void sysrq_handle_reboot(int key, struct pt_regs *pt_regs,
+static void sysrq_handle_reboot(int check_mask, int key, struct pt_regs *pt_regs,
 				struct tty_struct *tty)
 {
+	if (check_mask)
+		printk("<keyboard-invoked>\n");
+	else
+		printk("<non-keyboard-invoked: PID %d COMMAND %s>\n",
+			current->pid, current->comm);
 	lockdep_off();
 	local_irq_enable();
 	emergency_restart();
 }
 static struct sysrq_key_op sysrq_reboot_op = {
-	.handler	= sysrq_handle_reboot,
-	.help_msg	= "reBoot",
-	.action_msg	= "Resetting",
-	.enable_mask	= SYSRQ_ENABLE_BOOT,
+	.handler_with_mask	= sysrq_handle_reboot,
+	.help_msg		= "reBoot",
+	.action_msg		= "Resetting",
+	.enable_mask		= SYSRQ_ENABLE_BOOT,
 };
 
 static void sysrq_handle_sync(int key, struct pt_regs *pt_regs,
@@ -411,7 +416,10 @@ void __handle_sysrq(int key, struct pt_r
 			}
 			printk("%s\n", op_p->action_msg);
 			console_loglevel = orig_log_level;
-			op_p->handler(key, pt_regs, tty);
+			if (op_p->handler_with_mask)
+				op_p->handler_with_mask(check_mask, key, pt_regs, tty);
+			else
+				op_p->handler(key, pt_regs, tty);
 		} else {
 			printk("This sysrq operation is disabled.\n");
 		}
--- linux-2.6.18.i686/include/linux/sysrq.h.orig	2009-08-13 10:58:29.779925000 -0700
+++ linux-2.6.18.i686/include/linux/sysrq.h	2009-08-13 11:05:30.723865000 -0700
@@ -28,6 +28,7 @@ struct tty_struct;
 
 struct sysrq_key_op {
 	void (*handler)(int, struct pt_regs *, struct tty_struct *);
+	void (*handler_with_mask)(int check_mask, int, struct pt_regs *, struct tty_struct *);
 	char *help_msg;
 	char *action_msg;
 	int enable_mask;


Andrew Morton wrote:
>
> The changelog had no description of why the change should be applied
> and I was unable to understand the motivation from reading the patch,
> so I skipped it.
>
> Also the patch was wordwrapped and had all its tabs replaced by spaces.
>   


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sysrq: add debug message to reboot event
  2009-08-20  2:22     ` tina.yang
@ 2009-08-20 22:10       ` Andrew Morton
  2009-08-21  5:14         ` tina.yang
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-08-20 22:10 UTC (permalink / raw)
  To: tina.yang; +Cc: jkosina, linux-kernel

On Wed, 19 Aug 2009 19:22:16 -0700
"tina.yang" <tina.yang@oracle.com> wrote:

> Add debug message to detect keyboard vs non-keyboard triggered sysrq-b events.
> This is to assist postmortem debugging on complicated computing setup with
> large number of applications involved where reboot event had occurred, but 
> unclear of its origin.

I'm still struggling to understand the motivation for the change. 
There are a large number of ways in which a machine can be rebooted,
all the way down to a triple-fault.  So it seems fairly arbitrary to
add additional information to discriminate between just two of those
ways.

I assume that somewhere in your setup you have a script which does
`echo b /proc/sysrq-trigger' and it took ages to work out that this was
happening and you felt that having this code in place would have
helped you debug that problem, yes?

If so, I wonder what is the likelihood that someone else will have the
same problem and will find this change useful.

Perhaps we should do this for all sysrq events rather than just sysrq-b?


> --- linux-2.6.18.i686/drivers/char/sysrq.c.orig	2009-08-13 10:55:57.526459000 -0700
> +++ linux-2.6.18.i686/drivers/char/sysrq.c	2009-08-13 10:58:10.798739000 -0700

2.6.18 is truly ancient and this patch doesn't apply at all to
current development kernels.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] sysrq: add debug message to reboot event
  2009-08-20 22:10       ` Andrew Morton
@ 2009-08-21  5:14         ` tina.yang
  0 siblings, 0 replies; 6+ messages in thread
From: tina.yang @ 2009-08-21  5:14 UTC (permalink / raw)
  To: Andrew Morton; +Cc: jkosina, linux-kernel

Andrew Morton wrote:
> On Wed, 19 Aug 2009 19:22:16 -0700
> "tina.yang" <tina.yang@oracle.com> wrote:
>
>   
>> Add debug message to detect keyboard vs non-keyboard triggered sysrq-b events.
>> This is to assist postmortem debugging on complicated computing setup with
>> large number of applications involved where reboot event had occurred, but 
>> unclear of its origin.
>>     
>
> I'm still struggling to understand the motivation for the change. 
> There are a large number of ways in which a machine can be rebooted,
> all the way down to a triple-fault.  So it seems fairly arbitrary to
> add additional information to discriminate between just two of those
> ways.
>
> I assume that somewhere in your setup you have a script which does
> `echo b /proc/sysrq-trigger' and it took ages to work out that this was
> happening and you felt that having this code in place would have
> helped you debug that problem, yes?
>
> If so, I wonder what is the likelihood that someone else will have the
> same problem and will find this change useful.
>   
    Exactly what you've described except we are debugging someone else's
    (customers') setup remotely without familiarity of their environment
    and often with a time constraint.  The setup usually involves
    large database clusters and who knows what else are running there. 
    I think people in a comparable environment can benefit from a little
    more debug help from the system as we can.

> Perhaps we should do this for all sysrq events rather than just sysrq-b?
>
>
>   
    From a system perspective, I agree generalization of message handling is
    better.  The most critical one we have experienced is the reboot event.

>> --- linux-2.6.18.i686/drivers/char/sysrq.c.orig	2009-08-13 10:55:57.526459000 -0700
>> +++ linux-2.6.18.i686/drivers/char/sysrq.c	2009-08-13 10:58:10.798739000 -0700
>>     
>
> 2.6.18 is truly ancient and this patch doesn't apply at all to
> current development kernels.
>
>   
    Sorry grabbed the wrong one.

--- linux-2.6.git/drivers/char/sysrq.c.orig	2009-08-13 15:43:34.000000000 -0700
+++ linux-2.6.git/drivers/char/sysrq.c	2009-08-20 20:40:50.287922000 -0700
@@ -135,17 +135,22 @@ static struct sysrq_key_op sysrq_crash_o
 	.enable_mask	= SYSRQ_ENABLE_DUMP,
 };
 
-static void sysrq_handle_reboot(int key, struct tty_struct *tty)
+static void sysrq_handle_reboot(int check_mask, int key, struct tty_struct *tty)
 {
+	if (check_mask)
+		printk("<keyboard-invoked>\n");
+	else
+		printk("<non-keyboard-invoked: PID %d COMMAND %s>\n",
+			current->pid, current->comm);
 	lockdep_off();
 	local_irq_enable();
 	emergency_restart();
 }
 static struct sysrq_key_op sysrq_reboot_op = {
-	.handler	= sysrq_handle_reboot,
-	.help_msg	= "reBoot",
-	.action_msg	= "Resetting",
-	.enable_mask	= SYSRQ_ENABLE_BOOT,
+	.handler_with_mask	= sysrq_handle_reboot,
+	.help_msg		= "reBoot",
+	.action_msg		= "Resetting",
+	.enable_mask		= SYSRQ_ENABLE_BOOT,
 };
 
 static void sysrq_handle_sync(int key, struct tty_struct *tty)
@@ -509,7 +514,10 @@ void __handle_sysrq(int key, struct tty_
 		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
 			printk("%s\n", op_p->action_msg);
 			console_loglevel = orig_log_level;
-			op_p->handler(key, tty);
+			if (op_p->handler_with_mask)
+				op_p->handler_with_mask(check_mask, key, tty);
+			else
+				op_p->handler(key, tty);
 		} else {
 			printk("This sysrq operation is disabled.\n");
 		}
--- linux-2.6.git/include/linux/sysrq.h.orig	2009-08-13 20:53:48.085786000 -0700
+++ linux-2.6.git/include/linux/sysrq.h	2009-08-13 20:54:12.625756000 -0700
@@ -32,6 +32,7 @@ struct tty_struct;
 
 struct sysrq_key_op {
 	void (*handler)(int, struct tty_struct *);
+	void (*handler_with_mask)(int, int, struct tty_struct *);
 	char *help_msg;
 	char *action_msg;
 	int enable_mask;


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2009-08-21  5:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-17 19:16 [PATCH] sysrq: add debug message to reboot event tina.yang
2009-08-18 11:40 ` Jiri Kosina
2009-08-19 20:58   ` Andrew Morton
2009-08-20  2:22     ` tina.yang
2009-08-20 22:10       ` Andrew Morton
2009-08-21  5:14         ` tina.yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox