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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

Hi Konrad:
        Thanks for your review.

On 2016年10月01日 02:07, Konrad Rzeszutek Wilk wrote:
> 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.


When serial port driver works in the poll mode, it will set a regular
timer to deal with all input key and keyhandler(e,g dump_timerq()) will
run in the timer handler.

> 
>> 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.

I meant the serial port timer handler here which calls keyhandler
will run long time, no APIC timer interrupt will arrive to trigger timer
softirq and feed watchdog during this procedure. Because there is no
chance to program timer interrupt before completing all timer handlers
in this case.

>
>> 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).

In this case, APIC timer interrupt handler apic_timer_interrupt()
triggers timer softirq and runs all expired timer handlers in timer softirq.

> 
> 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. 

I want to make sure serial port timer handler doesn't run long time and
not affect feed dog operation.

> 
> 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?)

Do you mean to feed nmi watchdog in the keyhandler directly?

> 
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> 
> 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
>>


-- 
Best regards
Tianyu Lan

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