[PATCH] reduce side effects of handling '*' debug key

[PATCH] reduce side effects of handling '*' debug key

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

NMI watchdog should be suppressed, and softirqs should be handled at
least in the non-IRQ handler portion (they obviously must not be
processed in IRQ context).

The (slightly more involved) 4.0 variant of this patch is also
attached.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -444,16 +444,21 @@ static void run_all_nonirq_keyhandlers(u
     struct keyhandler *h;
     int k;
 
-    console_start_log_everything();
+    watchdog_disable();
+    console_start_sync();
+
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
+        process_pending_softirqs();
         h = key_table[k];
         if ( (h == NULL) || !h->diagnostic || h->irq_callback )
             continue;
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.fn)(k);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 }
 
 static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
@@ -464,10 +469,12 @@ static void run_all_keyhandlers(unsigned
     struct keyhandler *h;
     int k;
 
+    watchdog_disable();
+    console_start_sync();
+
     printk("'%c' pressed -> firing all diagnostic keyhandlers\n", key);
 
     /* Fire all the IRQ-context diangostic keyhandlers now */
-    console_start_log_everything();
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
         h = key_table[k];
@@ -476,7 +483,9 @@ static void run_all_keyhandlers(unsigned
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.irq_fn)(k, regs);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 
     /* Trigger the others from a tasklet in non-IRQ context */
     tasklet_schedule(&run_all_keyhandlers_tasklet);




[-- Attachment #2: keyhandler-relax.patch --]
[-- Type: text/plain, Size: 1797 bytes --]

NMI watchdog should be suppressed, and softirqs should be handled at
least in the non-IRQ handler portion (they obviously must not be
processed in IRQ context).

The (slightly more involved) 4.0 variant of this patch is also
attached.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -444,16 +444,21 @@ static void run_all_nonirq_keyhandlers(u
     struct keyhandler *h;
     int k;
 
-    console_start_log_everything();
+    watchdog_disable();
+    console_start_sync();
+
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
+        process_pending_softirqs();
         h = key_table[k];
         if ( (h == NULL) || !h->diagnostic || h->irq_callback )
             continue;
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.fn)(k);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 }
 
 static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
@@ -464,10 +469,12 @@ static void run_all_keyhandlers(unsigned
     struct keyhandler *h;
     int k;
 
+    watchdog_disable();
+    console_start_sync();
+
     printk("'%c' pressed -> firing all diagnostic keyhandlers\n", key);
 
     /* Fire all the IRQ-context diangostic keyhandlers now */
-    console_start_log_everything();
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
         h = key_table[k];
@@ -476,7 +483,9 @@ static void run_all_keyhandlers(unsigned
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.irq_fn)(k, regs);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 
     /* Trigger the others from a tasklet in non-IRQ context */
     tasklet_schedule(&run_all_keyhandlers_tasklet);

[-- Attachment #3: 4.0-keyhandler-relax.patch --]
[-- Type: text/plain, Size: 2889 bytes --]

NMI watchdog should be suppressed, and softirqs should be handled at
least in the non-IRQ handler portion (they obviously must not be
processed in IRQ context). Since softirqs, other than in 4.1, get
handled in a tasklet, we need to make sure we don't nest softirq
execution, which requires a new clone of process_pending_softirqs().

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -375,16 +375,21 @@ static void run_all_nonirq_keyhandlers(u
     struct keyhandler *h;
     int k;
 
-    console_start_log_everything();
+    watchdog_disable();
+    console_start_sync();
+
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
+        process_pending_softirqs_nested();
         h = key_table[k];
         if ( (h == NULL) || !h->diagnostic || h->irq_callback )
             continue;
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.fn)(k);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 }
 
 static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
@@ -395,10 +400,12 @@ static void run_all_keyhandlers(unsigned
     struct keyhandler *h;
     int k;
 
+    watchdog_disable();
+    console_start_sync();
+
     printk("'%c' pressed -> firing all diagnostic keyhandlers\n", key);
 
     /* Fire all the IRQ-context diangostic keyhandlers now */
-    console_start_log_everything();
     for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
     {
         h = key_table[k];
@@ -407,7 +414,9 @@ static void run_all_keyhandlers(unsigned
         printk("[%c: %s]\n", k, h->desc);
         (*h->u.irq_fn)(k, regs);
     }
-    console_end_log_everything();
+
+    console_end_sync();
+    watchdog_enable();
 
     /* Trigger the others from a tasklet in non-IRQ context */
     tasklet_schedule(&run_all_keyhandlers_tasklet);
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -54,6 +54,16 @@ void process_pending_softirqs(void)
     __do_softirq(1ul<<SCHEDULE_SOFTIRQ);
 }
 
+void process_pending_softirqs_nested(void)
+{
+    ASSERT(!in_irq() && local_irq_is_enabled());
+    /*
+     * Do not enter scheduler as it can preempt the calling context,
+     * and do not run tasklets as we're running one currently.
+     */
+    __do_softirq((1ul<<SCHEDULE_SOFTIRQ) | (1ul<<TASKLET_SOFTIRQ));
+}
+
 asmlinkage void do_softirq(void)
 {
     __do_softirq(0);
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -39,6 +39,8 @@ void raise_softirq(unsigned int nr);
  * Use this instead of do_softirq() when you do not want to be preempted.
  */
 void process_pending_softirqs(void);
+/* ... and use this instead when running inside a tasklet. */
+void process_pending_softirqs_nested(void);
 
 /*
  * TASKLETS -- dynamically-allocatable tasks run in softirq context

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] reduce side effects of handling '*' debug key

[Keir Fraser]

On 14/12/2010 14:46, "Jan Beulich" <JBeulich@novell.com> wrote:

> NMI watchdog should be suppressed, and softirqs should be handled at
> least in the non-IRQ handler portion (they obviously must not be
> processed in IRQ context).

Why do you add calls to watchdog_disable() --- The process_softirqs() call
you add is intended to be sufficient to keep timers being handled in a
timely fashion, is it not?

I can see why you remove console_start_log_everything(), since that is
handled from handle_keypress(), but why do you add calls to
console_start_sync()?

Personally I think that if you have justifiable reason to place the console
in synchronous mode, and protect yourself from the NMI watchdog, these calls
should be added to handle_keypress() for all keyhandlers to enjoy.

 -- Keir

> The (slightly more involved) 4.0 variant of this patch is also
> attached.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -444,16 +444,21 @@ static void run_all_nonirq_keyhandlers(u
>      struct keyhandler *h;
>      int k;
>  
> -    console_start_log_everything();
> +    watchdog_disable();
> +    console_start_sync();
> +
>      for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
>      {
> +        process_pending_softirqs();
>          h = key_table[k];
>          if ( (h == NULL) || !h->diagnostic || h->irq_callback )
>              continue;
>          printk("[%c: %s]\n", k, h->desc);
>          (*h->u.fn)(k);
>      }
> -    console_end_log_everything();
> +
> +    console_end_sync();
> +    watchdog_enable();
>  }
>  
>  static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
> @@ -464,10 +469,12 @@ static void run_all_keyhandlers(unsigned
>      struct keyhandler *h;
>      int k;
>  
> +    watchdog_disable();
> +    console_start_sync();
> +
>      printk("'%c' pressed -> firing all diagnostic keyhandlers\n", key);
>  
>      /* Fire all the IRQ-context diangostic keyhandlers now */
> -    console_start_log_everything();
>      for ( k = 0; k < ARRAY_SIZE(key_table); k++ )
>      {
>          h = key_table[k];
> @@ -476,7 +483,9 @@ static void run_all_keyhandlers(unsigned
>          printk("[%c: %s]\n", k, h->desc);
>          (*h->u.irq_fn)(k, regs);
>      }
> -    console_end_log_everything();
> +
> +    console_end_sync();
> +    watchdog_enable();
>  
>      /* Trigger the others from a tasklet in non-IRQ context */
>      tasklet_schedule(&run_all_keyhandlers_tasklet);
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] reduce side effects of handling '*' debug key

[Jan Beulich]

>>> On 15.12.10 at 11:45, Keir Fraser <keir@xen.org> wrote:
> On 14/12/2010 14:46, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> NMI watchdog should be suppressed, and softirqs should be handled at
>> least in the non-IRQ handler portion (they obviously must not be
>> processed in IRQ context).
> 
> Why do you add calls to watchdog_disable() --- The process_softirqs() call
> you add is intended to be sufficient to keep timers being handled in a
> timely fashion, is it not?
> 
> I can see why you remove console_start_log_everything(), since that is
> handled from handle_keypress(), but why do you add calls to
> console_start_sync()?

I didn't actually pay attention to the duplication with
handle_keypress(). Instead, I was simply following what the 'd'
handler does, noting that calling console_start_sync() makes
calling console_start_log_everything() pointless.

> Personally I think that if you have justifiable reason to place the console
> in synchronous mode, and protect yourself from the NMI watchdog, these calls
> should be added to handle_keypress() for all keyhandlers to enjoy.

Protection from the NMI watchdog shouldn't be needed for
handlers not taking long, so I'm not certain it'd be useful to add
generally.

Similarly, for handlers not printing much, forcing the console
into synchronous mode doesn't seem appropriate to me.
Basically, each handler should know for itself (and
handle_keypress() should, as it does, only do what all
handler want in common).

Jan