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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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. Plus your
other patch fixed the respective issue only for one individual
handler, instead of generally.

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

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