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

[Keir Fraser <keir@xen.org>]

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