Re: [Xen-devel] [PATCH] Xen: Force non-irq keyhandler to be run in tasklet when receive a debugkey from serial port

["Lan, Tianyu" <tianyu.lan@xxxxxxxxx>]

On 10/24/2016 10:28 PM, Jan Beulich wrote:
> > > > On 24.10.16 at 16:01, <tianyu.lan@xxxxxxxxx> wrote:
> > On 10/24/2016 6:53 PM, Jan Beulich wrote:
> > > > > > On 22.10.16 at 13:23, <tianyu.lan@xxxxxxxxx> wrote:
> > > > __serial_rx() runs in either irq handler or timer handler and non-irq
> > > > keyhandler should not run in these contexts. So always force non-irq
> > > > keyhandler to run in tasklet when receive a debugkey from serial port
> > > >
> > > > Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> > > > ---
> > > >  xen/drivers/char/console.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > > > index b0f74ce..184b523 100644
> > > > --- a/xen/drivers/char/console.c
> > > > +++ b/xen/drivers/char/console.c
> > > > @@ -347,7 +347,7 @@ static void switch_serial_input(void)
> > > >  static void __serial_rx(char c, struct cpu_user_regs *regs)
> > > >  {
> > > >      if ( xen_rx )
> > > > -        return handle_keypress(c, regs, !in_irq());
> > > > +        return handle_keypress(c, regs, true);
> > > Together with one of your earlier patches having got reverted, I
> > > think we need to take a step back here instead of going back to
> > > what was requested to be changed from v2 of the original patch.
> > > In particular I assume that the problem you're trying to address is
> > > not limited to dump_timerq() - at least dump_runq() should be as
> > > problematic on many-CPU systems.
> > I think the issue here is that my previous patch commit
> > 610b4eda2c("keyhandler: rework process of nonirq keyhandler") makes
> > non-irq keyhandler run in irq context. This is caused by input param
> > "!in_irq()" which is false in irq context. handle_keypress() runs
> > keyhandler synchronically. This patch fixes the issue.
> Not really - your earlier patch only moved the !in_irq() check, i.e.
> things continued to run in the same context they always did
> _except_ for the one special case you cared about.
I supposed the special case you meant is to run keyhandler in timer 
handler. It's necessary to make any timer handler run in a short time 
otherwise it will trigger watchdog problem.

> Plus your
> other patch fixed the respective issue only for one individual
> handler, instead of generally.
So you think adding process_pending_softirqs() in the keyhandler isn't
general? But this is a common solution so far.


> > > I think (and I vaguely recall possibly having said so during earlier
> > > review) that dump functions the output of which depends on CPU
> > > count should get modeled after dump_registers(), and it might be
> > > worth abstracting this in keyhandler.c.
> > Yes, but this sounds like a new feature or framework rework rather than
> > a fix patch.
> In a way, sure. It's a more extensive fix, which would avoid
> someone else running into the same issue with another handler.
This seems a big change and a lot of dump function needs to rework, right?



> > > In any case quite likely the
> > > other patch of yours (which the one here basically modifies) may
> > > then also want to be reverted.
> > I think patch "timer: process softirq during dumping timer"
> > does right thing. The issue is triggered by previous patch.
> Well - the issue did not exist prior to both of your patches
> going in, and I think it would have continued to exist if the
> keyhandler rework patch alone had been reverted. (And I'm
> afraid anyway that "previous" is ambiguous here, as the timer
> handler change went in first.)
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel