Re: [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Sep 30, 2016 at 10:19:05AM +0800, Lan Tianyu wrote:
> Keyhandler may run for a long time in a timer handler on the large machine

I am bit lost.

You say 'timer handler' which will imply that there is some form
of 'init_timer' and 'set_timer' that would call the handle_keypress
function?
But I am not seeing it?

Or are you saying that when 'dump_timerq' is invoked?
If so please say that.

> with a lot of physical cpus(E,G keyhandler for dumping timer info) when serial

s/E,G/e.g.g/

> port driver works in the poll mode. When timer interrupt arrives, timer subsystem

s/poll mode/poll mode (via the exception mechanism)/

> runs all timer handlers before programming next timer interrupt. So if timer handler
> runs longer than time for watchdog timeout, the timer handler of watchdog will be

Ah, so this is if a guest has set a timer and we are executing it. Or we have
many of them to go through.

> blocked to feed watchdog and xen hypervisor panics. This patch is to fix the issue
> via always scheduling a tasklet to run keyhandler to avoid timer handler running
> too long.

You say "timer handler" again. But the timer handlers are executed via
timer_softirq_action (which is a softirq, aka triggered by IPI).

And the tasklet will mean that that it gets to be executed _after_ the
do_softirq is done (as softirq.h puts the low numbered ones first, such
as the TIMER_SOFTIRQ)?

So what I think you are saying is that you do not want the 'timer_softirq_action'
to be preempted by the 'dump_timerq' (or any other ones) which will
trip the watchdog timeout. 

If that is the case please put something to that affect in the
commit description.

That begs one question that should be probably answered in the commit
description:

Why can't the dump_timerq or any other keyhandler poke the watchdog
(expose nmi_timer_fn and call that?)

> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

Otherwise the mechanical parts of the patch look good.

> ---
>  xen/common/keyhandler.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 16de6e8..fce52d2 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -75,7 +75,9 @@ static struct keyhandler {
>  
>  static void keypress_action(unsigned long unused)
>  {
> -    handle_keypress(keypress_key, NULL);
> +    console_start_log_everything();
> +    key_table[keypress_key].fn(keypress_key);
> +    console_end_log_everything();
>  }
>  
>  static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
> @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct cpu_user_regs *regs)
>      if ( key >= ARRAY_SIZE(key_table) || !(h = &key_table[key])->fn )
>          return;
>  
> -    if ( !in_irq() || h->irq_callback )
> +    if ( h->irq_callback )
>      {
>          console_start_log_everything();
> -        h->irq_callback ? h->irq_fn(key, regs) : h->fn(key);
> +        h->irq_fn(key, regs);
>          console_end_log_everything();
>      }
>      else
> -- 
> 1.7.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel