Jan Beulich
2010-Dec-14 14:46 UTC
[Xen-devel] [PATCH] reduce side effects of handling ''*'' debug key
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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2010-Dec-15 10:45 UTC
Re: [Xen-devel] [PATCH] reduce side effects of handling ''*'' debug key
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Dec-15 10:59 UTC
Re: [Xen-devel] [PATCH] reduce side effects of handling ''*'' debug key
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel