[Resend PATCH 0/2] Xen: Fix Xen hypervisor panic during dumping timer info on huge machine.

[Resend PATCH 0/2] Xen: Fix Xen hypervisor panic during dumping timer info on huge machine.

[Lan Tianyu]

Resend because the patchset seems to miss xen devel maillist.

This patchset is to fix triggering NMI watchdog during dump timer info
on the huge machine with a mount of physical cpus. Detail please see
change log of Patch 1.

Previous discussion:
https://patchwork.kernel.org/patch/9328449/


Lan Tianyu (2):
  Xen/Keyhandler: Make keyhandler always run in tasklet
  Xen/timer: Process softirq during dumping timer info

 xen/common/keyhandler.c |    8 +++++---
 xen/common/timer.c      |    1 +
 2 files changed, 6 insertions(+), 3 deletions(-)


 LocalWords:  8f82fa7cd8f2407b92d6994a65084951cf28a247

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

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

[Lan Tianyu]

Keyhandler may run for a long time in a timer handler on the large machine
with a lot of physical cpus(E,G keyhandler for dumping timer info) when serial
port driver works in the poll mode. When timer interrupt arrives, timer subsystem
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
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.

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

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

[Konrad Rzeszutek Wilk]

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

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

[Lan Tianyu]

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


-- 
Best regards
Tianyu Lan

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

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

[Jan Beulich]

>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
> @@ -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 )

Please make subject/description reflect this: You don't _always_
force the use of the tasklet.

And then I don't think we want the debugkey sysctl get processed
asynchronously - the sysctl should complete only when the key has
been fully handled, in order to not interfere with a subsequent one
(namely the one retrieving the log buffer).

Jan


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

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

[Lan Tianyu]

On 2016年10月06日 20:52, Jan Beulich wrote:
>>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
>> @@ -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 )
> 
> Please make subject/description reflect this: You don't _always_
> force the use of the tasklet.

Ok. I also find register_irq_keyhandler() isn't called anywhere in
current code and that means none uses irq_callback. Can we remove it?

> 
> And then I don't think we want the debugkey sysctl get processed
> asynchronously - the sysctl should complete only when the key has
> been fully handled, in order to not interfere with a subsequent one
> (namely the one retrieving the log buffer).

We may introduce a new parameter for handle_keypress() to specify
whether it should schedule a tasklet to run keyhandler or not. For
sysctl case, it should be the later one.

-- 
Best regards
Tianyu Lan

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

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

[Jan Beulich]

>>> On 08.10.16 at 05:26, <tianyu.lan@intel.com> wrote:
> On 2016年10月06日 20:52, Jan Beulich wrote:
>>>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
>>> @@ -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 )
>> 
>> Please make subject/description reflect this: You don't _always_
>> force the use of the tasklet.
> 
> Ok. I also find register_irq_keyhandler() isn't called anywhere in
> current code and that means none uses irq_callback. Can we remove it?

No, please don't. The decision to have it despite there being no
current user was taken not so long ago. You may want to consider
doing some archeology before asking question like this.

>> And then I don't think we want the debugkey sysctl get processed
>> asynchronously - the sysctl should complete only when the key has
>> been fully handled, in order to not interfere with a subsequent one
>> (namely the one retrieving the log buffer).
> 
> We may introduce a new parameter for handle_keypress() to specify
> whether it should schedule a tasklet to run keyhandler or not. For
> sysctl case, it should be the later one.

Yes, that's probably the route to go.

Jan

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

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

[Konrad Rzeszutek Wilk]

On Sat, Oct 08, 2016 at 11:26:44AM +0800, Lan Tianyu wrote:
> On 2016年10月06日 20:52, Jan Beulich wrote:
> >>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
> >> @@ -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 )
> > 
> > Please make subject/description reflect this: You don't _always_
> > force the use of the tasklet.
> 
> Ok. I also find register_irq_keyhandler() isn't called anywhere in
> current code and that means none uses irq_callback. Can we remove it?

But it is. See IRQ_KEYHANDLER
> 
> > 
> > And then I don't think we want the debugkey sysctl get processed
> > asynchronously - the sysctl should complete only when the key has
> > been fully handled, in order to not interfere with a subsequent one
> > (namely the one retrieving the log buffer).
> 
> We may introduce a new parameter for handle_keypress() to specify
> whether it should schedule a tasklet to run keyhandler or not. For
> sysctl case, it should be the later one.
> 
> -- 
> Best regards
> Tianyu Lan

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

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

[Lan Tianyu]

On 2016年10月10日 21:55, Konrad Rzeszutek Wilk wrote:
> On Sat, Oct 08, 2016 at 11:26:44AM +0800, Lan Tianyu wrote:
>> On 2016年10月06日 20:52, Jan Beulich wrote:
>>>>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
>>>> @@ -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 )
>>>
>>> Please make subject/description reflect this: You don't _always_
>>> force the use of the tasklet.
>>
>> Ok. I also find register_irq_keyhandler() isn't called anywhere in
>> current code and that means none uses irq_callback. Can we remove it?
> 
> But it is. See IRQ_KEYHANDLER

Oh. Yes. Thanks for your information.

-- 
Best regards
Tianyu Lan

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

[Resend PATCH 2/2] Xen/timer: Process softirq during dumping timer info

[Lan Tianyu]

Dumping timer info may run for a long time on the huge machine with
a lot of physical cpus. To avoid triggering NMI watchdog, add
process_pending_softirqs() in the loop of dumping timer info.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 xen/common/timer.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/common/timer.c b/xen/common/timer.c
index 29a60a9..ab6bca0 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -530,6 +530,7 @@ static void dump_timerq(unsigned char key)
     {
         ts = &per_cpu(timers, i);
 
+        process_pending_softirqs();
         printk("CPU%02d:\n", i);
         spin_lock_irqsave(&ts->lock, flags);
         for ( j = 1; j <= GET_HEAP_SIZE(ts->heap); j++ )
-- 
1.7.1


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

Re: [Resend PATCH 2/2] Xen/timer: Process softirq during dumping timer info

[Konrad Rzeszutek Wilk]

On Fri, Sep 30, 2016 at 10:19:06AM +0800, Lan Tianyu wrote:
> Dumping timer info may run for a long time on the huge machine with
> a lot of physical cpus. To avoid triggering NMI watchdog, add
> process_pending_softirqs() in the loop of dumping timer info.


Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  xen/common/timer.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/common/timer.c b/xen/common/timer.c
> index 29a60a9..ab6bca0 100644
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -530,6 +530,7 @@ static void dump_timerq(unsigned char key)
>      {
>          ts = &per_cpu(timers, i);
>  
> +        process_pending_softirqs();
>          printk("CPU%02d:\n", i);
>          spin_lock_irqsave(&ts->lock, flags);
>          for ( j = 1; j <= GET_HEAP_SIZE(ts->heap); j++ )
> -- 
> 1.7.1
> 

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

Re: [Resend PATCH 2/2] Xen/timer: Process softirq during dumping timer info

[Jan Beulich]

>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -530,6 +530,7 @@ static void dump_timerq(unsigned char key)
>      {
>          ts = &per_cpu(timers, i);
>  
> +        process_pending_softirqs();
>          printk("CPU%02d:\n", i);
>          spin_lock_irqsave(&ts->lock, flags);
>          for ( j = 1; j <= GET_HEAP_SIZE(ts->heap); j++ )

Hmm - is that enough when there are many timers on one CPU? But
well, adding something inside the lock region would of course make
things quite a bit harder, so I guess this has to be enough for now.

Jan


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

Re: [Resend PATCH 2/2] Xen/timer: Process softirq during dumping timer info

[Lan Tianyu]

On 2016年10月06日 20:56, Jan Beulich wrote:
>>>> On 30.09.16 at 04:19, <tianyu.lan@intel.com> wrote:
>> --- a/xen/common/timer.c
>> +++ b/xen/common/timer.c
>> @@ -530,6 +530,7 @@ static void dump_timerq(unsigned char key)
>>      {
>>          ts = &per_cpu(timers, i);
>>  
>> +        process_pending_softirqs();
>>          printk("CPU%02d:\n", i);
>>          spin_lock_irqsave(&ts->lock, flags);
>>          for ( j = 1; j <= GET_HEAP_SIZE(ts->heap); j++ )
> 
> Hmm - is that enough when there are many timers on one CPU? But
> well, adding something inside the lock region would of course make
> things quite a bit harder, so I guess this has to be enough for now.
> 

Yes, it's hard to add process_pending_softirqs() under lock just like
you said. I search init_timer() and there are 28 callers. Printing 28
lines of timer info is supposed to last a brief of time.

-- 
Best regards
Tianyu Lan

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