| Message ID | 1475201946-31898-2-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
>>> 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
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 >>
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.
>>> 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
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
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.
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
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(-)