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 17:03, <tianyu.lan@xxxxxxxxx> wrote:

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

No - it fixes just the one handler where you do that addition. As
already said, you for instance leave dump_runq() unfixed, yet I
believe it has the same issue.

> But this is a common solution so far.

It's a solution which has been used in a few special cases; I
wouldn't call this a "common solution", and I know I had
expressed this already during review of your original series.

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

I'm not sure. In particular I didn't go check which dump functions
have their output really scale with CPU count.

Jan

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