LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH -mm] extend sysrq-p functionality to cover all CPUs
@ 2008-03-10  2:14 Rik van Riel
  2008-03-10  5:47 ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Rik van Riel @ 2008-03-10  2:14 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel, lwoodman

SysRP-P is not all that useful on SMP systems, since the sysrq
irq rarely ends up on the CPU that we actually want to investigate.

This patch extends sysrq-p to print a backtrace for every CPU,
not just the lucky one that gets the sysrq irq.  With this patch,
"echo p > /proc/sysrq-trigger" does something useful.

Signed-off-by: Rik van Riel <riel@redhat.com>

diff -up linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu linux-2.6.25-rc3-mm1/drivers/char/sysrq.c
--- linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu	2008-03-09 20:22:17.000000000 -0400
+++ linux-2.6.25-rc3-mm1/drivers/char/sysrq.c	2008-03-09 21:54:02.000000000 -0400
@@ -196,11 +196,29 @@ static struct sysrq_key_op sysrq_showloc
 #define sysrq_showlocks_op (*(struct sysrq_key_op *)0)
 #endif
 
+static DEFINE_SPINLOCK(show_lock);
+static void showacpu(void *dummy)
+{
+	struct pt_regs *regs = get_irq_regs();
+
+	spin_lock(&show_lock);
+	printk("CPU%d:\n", smp_processor_id());
+	show_stack(NULL, NULL);
+	spin_unlock(&show_lock);
+}
+static void sysrq_showregs_othercpus(struct work_struct *dummy)
+{
+	smp_call_function(showacpu, NULL, 0, 0);
+}
+static DECLARE_WORK(sysrq_showregs, sysrq_showregs_othercpus);
 static void sysrq_handle_showregs(int key, struct tty_struct *tty)
 {
 	struct pt_regs *regs = get_irq_regs();
-	if (regs)
+	if (regs) {
+		printk("CPU%d:\n", smp_processor_id());
 		show_regs(regs);
+	}
+	schedule_work(&sysrq_showregs);
 }
 static struct sysrq_key_op sysrq_showregs_op = {
 	.handler	= sysrq_handle_showregs,


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

* Re: [PATCH -mm] extend sysrq-p functionality to cover all CPUs
  2008-03-10  2:14 [PATCH -mm] extend sysrq-p functionality to cover all CPUs Rik van Riel
@ 2008-03-10  5:47 ` Andrew Morton
  2008-03-10 13:30   ` Rik van Riel
  2008-03-10 14:58   ` Larry Woodman
  0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2008-03-10  5:47 UTC (permalink / raw)
  To: Rik van Riel; +Cc: linux-kernel, lwoodman

On Sun, 9 Mar 2008 22:14:58 -0400 Rik van Riel <riel@surriel.com> wrote:

> SysRP-P is not all that useful on SMP systems, since the sysrq
> irq rarely ends up on the CPU that we actually want to investigate.
> 
> This patch extends sysrq-p to print a backtrace for every CPU,
> not just the lucky one that gets the sysrq irq.  With this patch,
> "echo p > /proc/sysrq-trigger" does something useful.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> 
> diff -up linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu linux-2.6.25-rc3-mm1/drivers/char/sysrq.c
> --- linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu	2008-03-09 20:22:17.000000000 -0400
> +++ linux-2.6.25-rc3-mm1/drivers/char/sysrq.c	2008-03-09 21:54:02.000000000 -0400
> @@ -196,11 +196,29 @@ static struct sysrq_key_op sysrq_showloc
>  #define sysrq_showlocks_op (*(struct sysrq_key_op *)0)
>  #endif
>  
> +static DEFINE_SPINLOCK(show_lock);
> +static void showacpu(void *dummy)
> +{
> +	struct pt_regs *regs = get_irq_regs();
> +
> +	spin_lock(&show_lock);
> +	printk("CPU%d:\n", smp_processor_id());
> +	show_stack(NULL, NULL);
> +	spin_unlock(&show_lock);
> +}
> +static void sysrq_showregs_othercpus(struct work_struct *dummy)
> +{
> +	smp_call_function(showacpu, NULL, 0, 0);
> +}
> +static DECLARE_WORK(sysrq_showregs, sysrq_showregs_othercpus);
>  static void sysrq_handle_showregs(int key, struct tty_struct *tty)
>  {
>  	struct pt_regs *regs = get_irq_regs();
> -	if (regs)
> +	if (regs) {
> +		printk("CPU%d:\n", smp_processor_id());
>  		show_regs(regs);
> +	}
> +	schedule_work(&sysrq_showregs);
>  }
>  static struct sysrq_key_op sysrq_showregs_op = {
>  	.handler	= sysrq_handle_showregs,

Doesn't everyone have a copy of this somewhere? ;)

However it does have the downside that info can scroll away on large cpu
counts.  Maybe it should be a new sysrq command?


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

* Re: [PATCH -mm] extend sysrq-p functionality to cover all CPUs
  2008-03-10  5:47 ` Andrew Morton
@ 2008-03-10 13:30   ` Rik van Riel
  2008-03-10 16:24     ` Andrew Morton
  2008-03-10 14:58   ` Larry Woodman
  1 sibling, 1 reply; 6+ messages in thread
From: Rik van Riel @ 2008-03-10 13:30 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, lwoodman

On Sun, 9 Mar 2008 22:47:59 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sun, 9 Mar 2008 22:14:58 -0400 Rik van Riel <riel@surriel.com> wrote:
> 
> > SysRP-P is not all that useful on SMP systems, since the sysrq
> > irq rarely ends up on the CPU that we actually want to investigate.

> Doesn't everyone have a copy of this somewhere? ;)

Yes, but the old version of the patch calls on_each_cpu from
sysrq context, which is illegal since it could cause a deadlock.
 
> However it does have the downside that info can scroll away on large cpu
> counts.  Maybe it should be a new sysrq command?

It used to be sysrq-w in the patches that everybody has, but that
letter got taken for sysrq_showstate_blocked_op.

Only sysrq h, j, l, y and z are still free.  H is needed for help,
leaving just j, l, y and z.

I can see your point about overflowing the screen, however just
sysrq-p seems like a waste to have because it will probably not
print anything useful on a large CPU system...

If you still want it to be a separate letter, just let me know
which one of the last four I should take.

-- 
All rights reversed.

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

* Re: [PATCH -mm] extend sysrq-p functionality to cover all CPUs
  2008-03-10  5:47 ` Andrew Morton
  2008-03-10 13:30   ` Rik van Riel
@ 2008-03-10 14:58   ` Larry Woodman
  1 sibling, 0 replies; 6+ messages in thread
From: Larry Woodman @ 2008-03-10 14:58 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Rik van Riel, linux-kernel

Andrew Morton wrote:

>On Sun, 9 Mar 2008 22:14:58 -0400 Rik van Riel <riel@surriel.com> wrote:
>
>  
>
>>SysRP-P is not all that useful on SMP systems, since the sysrq
>>irq rarely ends up on the CPU that we actually want to investigate.
>>
>>This patch extends sysrq-p to print a backtrace for every CPU,
>>not just the lucky one that gets the sysrq irq.  With this patch,
>>"echo p > /proc/sysrq-trigger" does something useful.
>>
>>Signed-off-by: Rik van Riel <riel@redhat.com>
>>
>>diff -up linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu linux-2.6.25-rc3-mm1/drivers/char/sysrq.c
>>--- linux-2.6.25-rc3-mm1/drivers/char/sysrq.c.multicpu	2008-03-09 20:22:17.000000000 -0400
>>+++ linux-2.6.25-rc3-mm1/drivers/char/sysrq.c	2008-03-09 21:54:02.000000000 -0400
>>@@ -196,11 +196,29 @@ static struct sysrq_key_op sysrq_showloc
>> #define sysrq_showlocks_op (*(struct sysrq_key_op *)0)
>> #endif
>> 
>>+static DEFINE_SPINLOCK(show_lock);
>>+static void showacpu(void *dummy)
>>+{
>>+	struct pt_regs *regs = get_irq_regs();
>>+
>>+	spin_lock(&show_lock);
>>+	printk("CPU%d:\n", smp_processor_id());
>>+	show_stack(NULL, NULL);
>>+	spin_unlock(&show_lock);
>>+}
>>+static void sysrq_showregs_othercpus(struct work_struct *dummy)
>>+{
>>+	smp_call_function(showacpu, NULL, 0, 0);
>>+}
>>+static DECLARE_WORK(sysrq_showregs, sysrq_showregs_othercpus);
>> static void sysrq_handle_showregs(int key, struct tty_struct *tty)
>> {
>> 	struct pt_regs *regs = get_irq_regs();
>>-	if (regs)
>>+	if (regs) {
>>+		printk("CPU%d:\n", smp_processor_id());
>> 		show_regs(regs);
>>+	}
>>+	schedule_work(&sysrq_showregs);
>> }
>> static struct sysrq_key_op sysrq_showregs_op = {
>> 	.handler	= sysrq_handle_showregs,
>>    
>>
>
>Doesn't everyone have a copy of this somewhere? ;)
>

Yes, but we use W instead of P.  Dont you want to keep the old AlsSysrq 
P and add
a new SPM version using a defferent letter ?

Larry

>
>However it does have the downside that info can scroll away on large cpu
>counts.  Maybe it should be a new sysrq command?
>
>  
>



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

* Re: [PATCH -mm] extend sysrq-p functionality to cover all CPUs
  2008-03-10 13:30   ` Rik van Riel
@ 2008-03-10 16:24     ` Andrew Morton
  2008-03-10 17:03       ` Rik van Riel
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2008-03-10 16:24 UTC (permalink / raw)
  To: Rik van Riel; +Cc: linux-kernel, lwoodman

On Mon, 10 Mar 2008 09:30:24 -0400 Rik van Riel <riel@surriel.com> wrote:

> On Sun, 9 Mar 2008 22:47:59 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Sun, 9 Mar 2008 22:14:58 -0400 Rik van Riel <riel@surriel.com> wrote:
> > 
> > > SysRP-P is not all that useful on SMP systems, since the sysrq
> > > irq rarely ends up on the CPU that we actually want to investigate.
> 
> > Doesn't everyone have a copy of this somewhere? ;)
> 
> Yes, but the old version of the patch calls on_each_cpu from
> sysrq context, which is illegal since it could cause a deadlock.

My version did queue_work(), iirc, but I seem to have lost it.

I have an old patch here from Brice Goglin which does it via an NMI, but
that's arch-specific, I guess.

afaict, we _could_ implement an IPI call which isn't deadlockable (on x86,
at least) if we were to make it an asynchronous one.

Even if the hardware doesn't support queueing, this could still be done in
software via suitable locking and software queueing.

Whatever.

> > However it does have the downside that info can scroll away on large cpu
> > counts.  Maybe it should be a new sysrq command?
> 
> It used to be sysrq-w in the patches that everybody has, but that
> letter got taken for sysrq_showstate_blocked_op.
> 
> Only sysrq h, j, l, y and z are still free.  H is needed for help,
> leaving just j, l, y and z.
> 
> I can see your point about overflowing the screen,

iirc, this was the reason for not doing this years ago.  I don't know how
real that is.

> however just
> sysrq-p seems like a waste to have because it will probably not
> print anything useful on a large CPU system...

Yeah, sometimes it helps to keep hitting it until you get the right CPU,
but that rather depends on interrupt distribution.

It would be neat to suppress the trace for idle CPUs.  I don't _think_
there's a need to display the trace for idle CPUs in sysrq-P?

> If you still want it to be a separate letter, just let me know
> which one of the last four I should take.

l :)

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

* Re: [PATCH -mm] extend sysrq-p functionality to cover all CPUs
  2008-03-10 16:24     ` Andrew Morton
@ 2008-03-10 17:03       ` Rik van Riel
  0 siblings, 0 replies; 6+ messages in thread
From: Rik van Riel @ 2008-03-10 17:03 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, lwoodman

On Mon, 10 Mar 2008 09:24:13 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> > however just
> > sysrq-p seems like a waste to have because it will probably not
> > print anything useful on a large CPU system...
> 
> Yeah, sometimes it helps to keep hitting it until you get the right CPU,
> but that rather depends on interrupt distribution.
> 
> It would be neat to suppress the trace for idle CPUs.  I don't _think_
> there's a need to display the trace for idle CPUs in sysrq-P?
> 
> > If you still want it to be a separate letter, just let me know
> > which one of the last four I should take.
> 
> l :)

OK, I'll do that.  I will also change the show_lock to spinlock_irq_save
just in case sysrq-p gets invoked from multiple cpus simultaneously.

I'll send you the updated patch in a separate email, so the changelog
and commit logs do not have this discussion.


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

end of thread, other threads:[~2008-03-10 17:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-10  2:14 [PATCH -mm] extend sysrq-p functionality to cover all CPUs Rik van Riel
2008-03-10  5:47 ` Andrew Morton
2008-03-10 13:30   ` Rik van Riel
2008-03-10 16:24     ` Andrew Morton
2008-03-10 17:03       ` Rik van Riel
2008-03-10 14:58   ` Larry Woodman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).