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

2016-10-10 Thread 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,  wrote:
 @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct 
 cpu_user_regs *regs)
  if ( key >= ARRAY_SIZE(key_table) || !(h = _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

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

2016-10-10 Thread 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,  wrote:
> >> @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct 
> >> cpu_user_regs *regs)
> >>  if ( key >= ARRAY_SIZE(key_table) || !(h = _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: [Xen-devel] [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

2016-10-10 Thread Jan Beulich 
>>> On 08.10.16 at 05:26,  wrote:
> On 2016年10月06日 20:52, Jan Beulich wrote:
> On 30.09.16 at 04:19,  wrote:
>>> @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct 
> cpu_user_regs *regs)
>>>  if ( key >= ARRAY_SIZE(key_table) || !(h = _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: [Xen-devel] [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

2016-10-07 Thread Lan Tianyu 
On 2016年10月06日 20:52, Jan Beulich wrote:
 On 30.09.16 at 04:19,  wrote:
>> @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct 
>> cpu_user_regs *regs)
>>  if ( key >= ARRAY_SIZE(key_table) || !(h = _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: [Xen-devel] [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

2016-10-07 Thread 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 
> 
> 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 = _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: [Xen-devel] [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

2016-10-06 Thread Jan Beulich 
>>> On 30.09.16 at 04:19,  wrote:
> @@ -87,10 +89,10 @@ void handle_keypress(unsigned char key, struct 
> cpu_user_regs *regs)
>  if ( key >= ARRAY_SIZE(key_table) || !(h = _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: [Xen-devel] [Resend PATCH 1/2] Xen/Keyhandler: Make keyhandler always run in tasklet

2016-09-30 Thread 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 

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 = _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

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

2016-09-29 Thread 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 
---
 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 = _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