xen/evtchn and forced threaded irq

xen/evtchn and forced threaded irq

[Julien Grall]

Hi all,

I have been looking at using Linux RT in Dom0. Once the guest is started,
the console is ending to have a lot of warning (see trace below).

After some investigation, this is because the irq handler will now be threaded.
I can reproduce the same error with the vanilla Linux when passing the option
'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
not RT support).

FWIW, the interrupt for port 6 is used to for the guest to communicate with
xenstore.

From my understanding, this is happening because the interrupt handler is now
run in a thread. So we can have the following happening.

   Interrupt context            |     Interrupt thread
                                |  
   receive interrupt port 6     |
   clear the evtchn port        |
   set IRQF_RUNTHREAD	        |
   kick interrupt thread        |
                                |    clear IRQF_RUNTHREAD
                                |    call evtchn_interrupt
   receive interrupt port 6     |
   clear the evtchn port        |
   set IRQF_RUNTHREAD           |
   kick interrupt thread        |
                                |    disable interrupt port 6
                                |    evtchn->enabled = false
                                |    [....]
                                |
                                |    *** Handling the second interrupt ***
                                |    clear IRQF_RUNTHREAD
                                |    call evtchn_interrupt
                                |    WARN(...)

I am not entirely sure how to fix this. I have two solutions in mind:

1) Prevent the interrupt handler to be threaded. We would also need to
switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.

2) Remove the warning

None of them are ideals. Do you have an opionion/better suggestion?

[  127.192087] Interrupt for port 6, but apparently not enabled; per-user 0000000078d39c7f
[  127.200333] WARNING: CPU: 0 PID: 2553 at drivers/xen/evtchn.c:167 evtchn_interrupt+0xfc/0x120
[  127.208799] Modules linked in:
[  127.211939] CPU: 0 PID: 2553 Comm: irq/52-evtchn:x Tainted: G        W
  5.0.0-rc7-00023-g2a3d41623699 #1257
[  127.222374] Hardware name: ARM Juno development board (r2) (DT)
[  127.228381] pstate: 40000005 (nZcv daif -PAN -UAO)
[  127.233256] pc : evtchn_interrupt+0xfc/0x120
[  127.237607] lr : evtchn_interrupt+0xfc/0x120
[  127.241952] sp : ffff000012d2bd60
[  127.245347] x29: ffff000012d2bd60 x28: ffff00001015d608
[  127.250741] x27: ffff8008b39de400 x26: ffff00001015d330
[  127.256137] x25: ffff00001015d2d4 x24: 0000000000000001
[  127.261532] x23: ffff00001015d570 x22: 0000000000000034
[  127.266926] x21: 0000000000000000 x20: ffff8008b7f02400
[  127.272322] x19: ffff8008b3aba000 x18: 0000000000000037
[  127.277717] x17: 0000000000000000 x16: 0000000000000000
[  127.283112] x15: 00000000fffffff0 x14: 0000000000000000
[  127.288507] x13: 3030303030207265 x12: 000000000000000c
[  127.293902] x11: ffff000010ea0a88 x10: 0000000000000000
[  127.299297] x9 : 00000000fffb9fff x8 : 0000000000000000
[  127.304701] x7 : 0000000000000001 x6 : ffff8008bac5c240
[  127.310087] x5 : ffff8008bac5c240 x4 : 0000000000000000
[  127.315483] x3 : ffff8008bac64708 x2 : ffff8008b39de400
[  127.320877] x1 : 893ac9b38837b800 x0 : 0000000000000000
[  127.326272] Call trace:
[  127.328802]  evtchn_interrupt+0xfc/0x120
[  127.332804]  irq_forced_thread_fn+0x38/0x98
[  127.337066]  irq_thread+0x190/0x238
[  127.340636]  kthread+0x134/0x138
[  127.343942]  ret_from_fork+0x10/0x1c
[  127.347593] ---[ end trace 1d3fa385877cc18b ]---


Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
> Hi all,
> 
> I have been looking at using Linux RT in Dom0. Once the guest is started,
> the console is ending to have a lot of warning (see trace below).
> 
> After some investigation, this is because the irq handler will now be threaded.
> I can reproduce the same error with the vanilla Linux when passing the option
> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
> not RT support).
> 
> FWIW, the interrupt for port 6 is used to for the guest to communicate with
> xenstore.
> 
> From my understanding, this is happening because the interrupt handler is now
> run in a thread. So we can have the following happening.
> 
>    Interrupt context            |     Interrupt thread
>                                 |  
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD	        |
>    kick interrupt thread        |
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD           |
>    kick interrupt thread        |
>                                 |    disable interrupt port 6
>                                 |    evtchn->enabled = false
>                                 |    [....]
>                                 |
>                                 |    *** Handling the second interrupt ***
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>                                 |    WARN(...)
> 
> I am not entirely sure how to fix this. I have two solutions in mind:
> 
> 1) Prevent the interrupt handler to be threaded. We would also need to
> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
> 
> 2) Remove the warning

I think access to evtchn->enabled is racy so (with or without the warning) we can't use it reliably.

Another alternative could be to queue the irq if !evtchn->enabled and handle it in evtchn_write() (which is where irq is supposed to be re-enabled).


-boris


> 
> None of them are ideals. Do you have an opionion/better suggestion?
> 
> [  127.192087] Interrupt for port 6, but apparently not enabled; per-user 0000000078d39c7f
> [  127.200333] WARNING: CPU: 0 PID: 2553 at drivers/xen/evtchn.c:167 evtchn_interrupt+0xfc/0x120
> [  127.208799] Modules linked in:
> [  127.211939] CPU: 0 PID: 2553 Comm: irq/52-evtchn:x Tainted: G        W
>   5.0.0-rc7-00023-g2a3d41623699 #1257
> [  127.222374] Hardware name: ARM Juno development board (r2) (DT)
> [  127.228381] pstate: 40000005 (nZcv daif -PAN -UAO)
> [  127.233256] pc : evtchn_interrupt+0xfc/0x120
> [  127.237607] lr : evtchn_interrupt+0xfc/0x120
> [  127.241952] sp : ffff000012d2bd60
> [  127.245347] x29: ffff000012d2bd60 x28: ffff00001015d608
> [  127.250741] x27: ffff8008b39de400 x26: ffff00001015d330
> [  127.256137] x25: ffff00001015d2d4 x24: 0000000000000001
> [  127.261532] x23: ffff00001015d570 x22: 0000000000000034
> [  127.266926] x21: 0000000000000000 x20: ffff8008b7f02400
> [  127.272322] x19: ffff8008b3aba000 x18: 0000000000000037
> [  127.277717] x17: 0000000000000000 x16: 0000000000000000
> [  127.283112] x15: 00000000fffffff0 x14: 0000000000000000
> [  127.288507] x13: 3030303030207265 x12: 000000000000000c
> [  127.293902] x11: ffff000010ea0a88 x10: 0000000000000000
> [  127.299297] x9 : 00000000fffb9fff x8 : 0000000000000000
> [  127.304701] x7 : 0000000000000001 x6 : ffff8008bac5c240
> [  127.310087] x5 : ffff8008bac5c240 x4 : 0000000000000000
> [  127.315483] x3 : ffff8008bac64708 x2 : ffff8008b39de400
> [  127.320877] x1 : 893ac9b38837b800 x0 : 0000000000000000
> [  127.326272] Call trace:
> [  127.328802]  evtchn_interrupt+0xfc/0x120
> [  127.332804]  irq_forced_thread_fn+0x38/0x98
> [  127.337066]  irq_thread+0x190/0x238
> [  127.340636]  kthread+0x134/0x138
> [  127.343942]  ret_from_fork+0x10/0x1c
> [  127.347593] ---[ end trace 1d3fa385877cc18b ]---
> 
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Boris,

Thank you for your answer.

On 20/02/2019 00:02, Boris Ostrovsky wrote:
> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>> Hi all,
>>
>> I have been looking at using Linux RT in Dom0. Once the guest is started,
>> the console is ending to have a lot of warning (see trace below).
>>
>> After some investigation, this is because the irq handler will now be threaded.
>> I can reproduce the same error with the vanilla Linux when passing the option
>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
>> not RT support).
>>
>> FWIW, the interrupt for port 6 is used to for the guest to communicate with
>> xenstore.
>>
>>  From my understanding, this is happening because the interrupt handler is now
>> run in a thread. So we can have the following happening.
>>
>>     Interrupt context            |     Interrupt thread
>>                                  |
>>     receive interrupt port 6     |
>>     clear the evtchn port        |
>>     set IRQF_RUNTHREAD	        |
>>     kick interrupt thread        |
>>                                  |    clear IRQF_RUNTHREAD
>>                                  |    call evtchn_interrupt
>>     receive interrupt port 6     |
>>     clear the evtchn port        |
>>     set IRQF_RUNTHREAD           |
>>     kick interrupt thread        |
>>                                  |    disable interrupt port 6
>>                                  |    evtchn->enabled = false
>>                                  |    [....]
>>                                  |
>>                                  |    *** Handling the second interrupt ***
>>                                  |    clear IRQF_RUNTHREAD
>>                                  |    call evtchn_interrupt
>>                                  |    WARN(...)
>>
>> I am not entirely sure how to fix this. I have two solutions in mind:
>>
>> 1) Prevent the interrupt handler to be threaded. We would also need to
>> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
>>
>> 2) Remove the warning
> 
> I think access to evtchn->enabled is racy so (with or without the warning) we can't use it reliably.

Thinking about it, it would not be the only issue. The ring is sized to contain 
only one instance of the same event. So if you receive twice the event, you may 
overflow the ring.

> 
> Another alternative could be to queue the irq if !evtchn->enabled and handle it in evtchn_write() (which is where irq is supposed to be re-enabled).
What do you mean by queue? Is it queueing in the ring? If so, wouldn't it be 
racy as described above?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Boris,

Thank you for your answer.

On 20/02/2019 00:02, Boris Ostrovsky wrote:
> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>> Hi all,
>>
>> I have been looking at using Linux RT in Dom0. Once the guest is started,
>> the console is ending to have a lot of warning (see trace below).
>>
>> After some investigation, this is because the irq handler will now be threaded.
>> I can reproduce the same error with the vanilla Linux when passing the option
>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
>> not RT support).
>>
>> FWIW, the interrupt for port 6 is used to for the guest to communicate with
>> xenstore.
>>
>>  From my understanding, this is happening because the interrupt handler is now
>> run in a thread. So we can have the following happening.
>>
>>     Interrupt context            |     Interrupt thread
>>                                  |
>>     receive interrupt port 6     |
>>     clear the evtchn port        |
>>     set IRQF_RUNTHREAD	        |
>>     kick interrupt thread        |
>>                                  |    clear IRQF_RUNTHREAD
>>                                  |    call evtchn_interrupt
>>     receive interrupt port 6     |
>>     clear the evtchn port        |
>>     set IRQF_RUNTHREAD           |
>>     kick interrupt thread        |
>>                                  |    disable interrupt port 6
>>                                  |    evtchn->enabled = false
>>                                  |    [....]
>>                                  |
>>                                  |    *** Handling the second interrupt ***
>>                                  |    clear IRQF_RUNTHREAD
>>                                  |    call evtchn_interrupt
>>                                  |    WARN(...)
>>
>> I am not entirely sure how to fix this. I have two solutions in mind:
>>
>> 1) Prevent the interrupt handler to be threaded. We would also need to
>> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
>>
>> 2) Remove the warning
> 
> I think access to evtchn->enabled is racy so (with or without the warning) we can't use it reliably.

Thinking about it, it would not be the only issue. The ring is sized to contain 
only one instance of the same event. So if you receive twice the event, you may 
overflow the ring.

> 
> Another alternative could be to queue the irq if !evtchn->enabled and handle it in evtchn_write() (which is where irq is supposed to be re-enabled).
What do you mean by queue? Is it queueing in the ring? If so, wouldn't it be 
racy as described above?

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 9:15 AM, Julien Grall wrote:
> Hi Boris,
>
> Thank you for your answer.
>
> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>> Hi all,
>>>
>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>> started,
>>> the console is ending to have a lot of warning (see trace below).
>>>
>>> After some investigation, this is because the irq handler will now
>>> be threaded.
>>> I can reproduce the same error with the vanilla Linux when passing
>>> the option
>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>> that has
>>> not RT support).
>>>
>>> FWIW, the interrupt for port 6 is used to for the guest to
>>> communicate with
>>> xenstore.
>>>
>>>  From my understanding, this is happening because the interrupt
>>> handler is now
>>> run in a thread. So we can have the following happening.
>>>
>>>     Interrupt context            |     Interrupt thread
>>>                                  |
>>>     receive interrupt port 6     |
>>>     clear the evtchn port        |
>>>     set IRQF_RUNTHREAD            |
>>>     kick interrupt thread        |
>>>                                  |    clear IRQF_RUNTHREAD
>>>                                  |    call evtchn_interrupt
>>>     receive interrupt port 6     |
>>>     clear the evtchn port        |
>>>     set IRQF_RUNTHREAD           |
>>>     kick interrupt thread        |
>>>                                  |    disable interrupt port 6
>>>                                  |    evtchn->enabled = false
>>>                                  |    [....]
>>>                                  |
>>>                                  |    *** Handling the second
>>> interrupt ***
>>>                                  |    clear IRQF_RUNTHREAD
>>>                                  |    call evtchn_interrupt
>>>                                  |    WARN(...)
>>>
>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>
>>> 1) Prevent the interrupt handler to be threaded. We would also need to
>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>> RT-Linux.
>>>
>>> 2) Remove the warning
>>
>> I think access to evtchn->enabled is racy so (with or without the
>> warning) we can't use it reliably.
>
> Thinking about it, it would not be the only issue. The ring is sized
> to contain only one instance of the same event. So if you receive
> twice the event, you may overflow the ring.

Hm... That's another argument in favor of "unthreading" the handler.

>
>>
>> Another alternative could be to queue the irq if !evtchn->enabled and
>> handle it in evtchn_write() (which is where irq is supposed to be
>> re-enabled).
> What do you mean by queue? Is it queueing in the ring? 


No, I was thinking about having a new structure for deferred interrupts.

-boris

> If so, wouldn't it be racy as described above?

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 20/02/2019 17:07, Boris Ostrovsky wrote:
> On 2/20/19 9:15 AM, Julien Grall wrote:
>> Hi Boris,
>>
>> Thank you for your answer.
>>
>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>> Hi all,
>>>>
>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>> started,
>>>> the console is ending to have a lot of warning (see trace below).
>>>>
>>>> After some investigation, this is because the irq handler will now
>>>> be threaded.
>>>> I can reproduce the same error with the vanilla Linux when passing
>>>> the option
>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>> that has
>>>> not RT support).
>>>>
>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>> communicate with
>>>> xenstore.
>>>>
>>>>   From my understanding, this is happening because the interrupt
>>>> handler is now
>>>> run in a thread. So we can have the following happening.
>>>>
>>>>      Interrupt context            |     Interrupt thread
>>>>                                   |
>>>>      receive interrupt port 6     |
>>>>      clear the evtchn port        |
>>>>      set IRQF_RUNTHREAD            |
>>>>      kick interrupt thread        |
>>>>                                   |    clear IRQF_RUNTHREAD
>>>>                                   |    call evtchn_interrupt
>>>>      receive interrupt port 6     |
>>>>      clear the evtchn port        |
>>>>      set IRQF_RUNTHREAD           |
>>>>      kick interrupt thread        |
>>>>                                   |    disable interrupt port 6
>>>>                                   |    evtchn->enabled = false
>>>>                                   |    [....]
>>>>                                   |
>>>>                                   |    *** Handling the second
>>>> interrupt ***
>>>>                                   |    clear IRQF_RUNTHREAD
>>>>                                   |    call evtchn_interrupt
>>>>                                   |    WARN(...)
>>>>
>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>
>>>> 1) Prevent the interrupt handler to be threaded. We would also need to
>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>> RT-Linux.
>>>>
>>>> 2) Remove the warning
>>>
>>> I think access to evtchn->enabled is racy so (with or without the
>>> warning) we can't use it reliably.
>>
>> Thinking about it, it would not be the only issue. The ring is sized
>> to contain only one instance of the same event. So if you receive
>> twice the event, you may overflow the ring.
> 
> Hm... That's another argument in favor of "unthreading" the handler.

I first thought it would be possible to unthread it. However, 
wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep, so this 
cannot be used in an interrupt context.

So I think "unthreading" the handler is not an option here.

> 
>>
>>>
>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>> handle it in evtchn_write() (which is where irq is supposed to be
>>> re-enabled).
>> What do you mean by queue? Is it queueing in the ring?
> 
> 
> No, I was thinking about having a new structure for deferred interrupts.

Hmmm, I am not entirely sure what would be the structure here. Could you expand 
your thinking?

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 1:05 PM, Julien Grall wrote:
> Hi,
>
> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>> Hi Boris,
>>>
>>> Thank you for your answer.
>>>
>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>> Hi all,
>>>>>
>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>> started,
>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>
>>>>> After some investigation, this is because the irq handler will now
>>>>> be threaded.
>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>> the option
>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>> that has
>>>>> not RT support).
>>>>>
>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>> communicate with
>>>>> xenstore.
>>>>>
>>>>>   From my understanding, this is happening because the interrupt
>>>>> handler is now
>>>>> run in a thread. So we can have the following happening.
>>>>>
>>>>>      Interrupt context            |     Interrupt thread
>>>>>                                   |
>>>>>      receive interrupt port 6     |
>>>>>      clear the evtchn port        |
>>>>>      set IRQF_RUNTHREAD            |
>>>>>      kick interrupt thread        |
>>>>>                                   |    clear IRQF_RUNTHREAD
>>>>>                                   |    call evtchn_interrupt
>>>>>      receive interrupt port 6     |
>>>>>      clear the evtchn port        |
>>>>>      set IRQF_RUNTHREAD           |
>>>>>      kick interrupt thread        |
>>>>>                                   |    disable interrupt port 6
>>>>>                                   |    evtchn->enabled = false
>>>>>                                   |    [....]
>>>>>                                   |
>>>>>                                   |    *** Handling the second
>>>>> interrupt ***
>>>>>                                   |    clear IRQF_RUNTHREAD
>>>>>                                   |    call evtchn_interrupt
>>>>>                                   |    WARN(...)
>>>>>
>>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>>
>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>> need to
>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>> RT-Linux.
>>>>>
>>>>> 2) Remove the warning
>>>>
>>>> I think access to evtchn->enabled is racy so (with or without the
>>>> warning) we can't use it reliably.
>>>
>>> Thinking about it, it would not be the only issue. The ring is sized
>>> to contain only one instance of the same event. So if you receive
>>> twice the event, you may overflow the ring.
>>
>> Hm... That's another argument in favor of "unthreading" the handler.
>
> I first thought it would be possible to unthread it. However,
> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
> so this cannot be used in an interrupt context.
>
> So I think "unthreading" the handler is not an option here.

That sounds like a different problem. I.e. there are two issues:
* threaded interrupts don't work properly (races, ring overflow)
* evtchn_interrupt() (threaded or not) has spin_lock(), which is not
going to work for RT

The first can be fixed by using non-threaded handlers.

>
>>
>>>
>>>>
>>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>> re-enabled).
>>> What do you mean by queue? Is it queueing in the ring?
>>
>>
>> No, I was thinking about having a new structure for deferred interrupts.
>
> Hmmm, I am not entirely sure what would be the structure here. Could
> you expand your thinking?

Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
implemented as a ring but not necessarily as Xen shared ring (if that's
what you were referring to).

-boris

Re: xen/evtchn and forced threaded irq

[Julien Grall]

(+ Andrew and Jan for feedback on the event channel interrupt)

Hi Boris,

Thank you for the your feedback.

On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
> On 2/20/19 1:05 PM, Julien Grall wrote:
>> Hi,
>>
>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>> Hi Boris,
>>>>
>>>> Thank you for your answer.
>>>>
>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>> started,
>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>
>>>>>> After some investigation, this is because the irq handler will now
>>>>>> be threaded.
>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>> the option
>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>> that has
>>>>>> not RT support).
>>>>>>
>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>> communicate with
>>>>>> xenstore.
>>>>>>
>>>>>>    From my understanding, this is happening because the interrupt
>>>>>> handler is now
>>>>>> run in a thread. So we can have the following happening.
>>>>>>
>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>                                    |
>>>>>>       receive interrupt port 6     |
>>>>>>       clear the evtchn port        |
>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>       kick interrupt thread        |
>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>                                    |    call evtchn_interrupt
>>>>>>       receive interrupt port 6     |
>>>>>>       clear the evtchn port        |
>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>       kick interrupt thread        |
>>>>>>                                    |    disable interrupt port 6
>>>>>>                                    |    evtchn->enabled = false
>>>>>>                                    |    [....]
>>>>>>                                    |
>>>>>>                                    |    *** Handling the second
>>>>>> interrupt ***
>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>                                    |    call evtchn_interrupt
>>>>>>                                    |    WARN(...)
>>>>>>
>>>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>>>
>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>> need to
>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>> RT-Linux.
>>>>>>
>>>>>> 2) Remove the warning
>>>>>
>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>> warning) we can't use it reliably.
>>>>
>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>> to contain only one instance of the same event. So if you receive
>>>> twice the event, you may overflow the ring.
>>>
>>> Hm... That's another argument in favor of "unthreading" the handler.
>>
>> I first thought it would be possible to unthread it. However,
>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>> so this cannot be used in an interrupt context.
>>
>> So I think "unthreading" the handler is not an option here.
> 
> That sounds like a different problem. I.e. there are two issues:
> * threaded interrupts don't work properly (races, ring overflow)
> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
> going to work for RT

I am afraid that's not correct, you can use spin_lock() in threaded 
interrupt handler.

> The first can be fixed by using non-threaded handlers.

The two are somewhat related, if you use a non-threaded handler then you 
are not going to help the RT case.

In general, the unthreaded solution should be used in the last resort.

>>
>>>
>>>>
>>>>>
>>>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>> re-enabled).
>>>> What do you mean by queue? Is it queueing in the ring?
>>>
>>>
>>> No, I was thinking about having a new structure for deferred interrupts.
>>
>> Hmmm, I am not entirely sure what would be the structure here. Could
>> you expand your thinking?
> 
> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
> implemented as a ring but not necessarily as Xen shared ring (if that's
> what you were referring to).

The underlying question is what happen if you miss an interrupt. Is it 
going to be ok? If no, then we have to record everything and can't 
kill/send an error to the user app because it is not its fault.

This means a FIFO would not be a viable. How do you size it? Static (i.e 
pre-defined) size is not going to be possible because you don't know how 
many interrupt you are going to receive before the thread handler runs. 
You can't make it grow dynamically as it make become quite big for the 
same reason.

Discussing with my team, a solution that came up would be to introduce 
one atomic field per event to record the number of event received. I 
will explore that solution tomorrow.

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 3:46 PM, Julien Grall wrote:
> (+ Andrew and Jan for feedback on the event channel interrupt)
>
> Hi Boris,
>
> Thank you for the your feedback.
>
> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>> Hi Boris,
>>>>>
>>>>> Thank you for your answer.
>>>>>
>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>> started,
>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>
>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>> be threaded.
>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>> the option
>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>> that has
>>>>>>> not RT support).
>>>>>>>
>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>> communicate with
>>>>>>> xenstore.
>>>>>>>
>>>>>>>    From my understanding, this is happening because the interrupt
>>>>>>> handler is now
>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>
>>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>>                                    |
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    disable interrupt port 6
>>>>>>>                                    |    evtchn->enabled = false
>>>>>>>                                    |    [....]
>>>>>>>                                    |
>>>>>>>                                    |    *** Handling the second
>>>>>>> interrupt ***
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>                                    |    WARN(...)
>>>>>>>
>>>>>>> I am not entirely sure how to fix this. I have two solutions in
>>>>>>> mind:
>>>>>>>
>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>> need to
>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>> RT-Linux.
>>>>>>>
>>>>>>> 2) Remove the warning
>>>>>>
>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>> warning) we can't use it reliably.
>>>>>
>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>> to contain only one instance of the same event. So if you receive
>>>>> twice the event, you may overflow the ring.
>>>>
>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>
>>> I first thought it would be possible to unthread it. However,
>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>> so this cannot be used in an interrupt context.
>>>
>>> So I think "unthreading" the handler is not an option here.
>>
>> That sounds like a different problem. I.e. there are two issues:
>> * threaded interrupts don't work properly (races, ring overflow)
>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>> going to work for RT
>
> I am afraid that's not correct, you can use spin_lock() in threaded
> interrupt handler.

In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
you yourself said above?)

>
>> The first can be fixed by using non-threaded handlers.
>
> The two are somewhat related, if you use a non-threaded handler then
> you are not going to help the RT case.
>
> In general, the unthreaded solution should be used in the last resort.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Another alternative could be to queue the irq if !evtchn->enabled
>>>>>> and
>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>> re-enabled).
>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>
>>>>
>>>> No, I was thinking about having a new structure for deferred
>>>> interrupts.
>>>
>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>> you expand your thinking?
>>
>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>> implemented as a ring but not necessarily as Xen shared ring (if that's
>> what you were referring to).
>
> The underlying question is what happen if you miss an interrupt. Is it
> going to be ok?

This I am not sure about. I thought yes since we are signaling the
process only once.

-boris

> If no, then we have to record everything and can't kill/send an error
> to the user app because it is not its fault.
>
> This means a FIFO would not be a viable. How do you size it? Static
> (i.e pre-defined) size is not going to be possible because you don't
> know how many interrupt you are going to receive before the thread
> handler runs. You can't make it grow dynamically as it make become
> quite big for the same reason.
>
> Discussing with my team, a solution that came up would be to introduce
> one atomic field per event to record the number of event received. I
> will explore that solution tomorrow.
>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 3:46 PM, Julien Grall wrote:
> (+ Andrew and Jan for feedback on the event channel interrupt)
>
> Hi Boris,
>
> Thank you for the your feedback.
>
> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>> Hi Boris,
>>>>>
>>>>> Thank you for your answer.
>>>>>
>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>> started,
>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>
>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>> be threaded.
>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>> the option
>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>> that has
>>>>>>> not RT support).
>>>>>>>
>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>> communicate with
>>>>>>> xenstore.
>>>>>>>
>>>>>>>    From my understanding, this is happening because the interrupt
>>>>>>> handler is now
>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>
>>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>>                                    |
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    disable interrupt port 6
>>>>>>>                                    |    evtchn->enabled = false
>>>>>>>                                    |    [....]
>>>>>>>                                    |
>>>>>>>                                    |    *** Handling the second
>>>>>>> interrupt ***
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>                                    |    WARN(...)
>>>>>>>
>>>>>>> I am not entirely sure how to fix this. I have two solutions in
>>>>>>> mind:
>>>>>>>
>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>> need to
>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>> RT-Linux.
>>>>>>>
>>>>>>> 2) Remove the warning
>>>>>>
>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>> warning) we can't use it reliably.
>>>>>
>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>> to contain only one instance of the same event. So if you receive
>>>>> twice the event, you may overflow the ring.
>>>>
>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>
>>> I first thought it would be possible to unthread it. However,
>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>> so this cannot be used in an interrupt context.
>>>
>>> So I think "unthreading" the handler is not an option here.
>>
>> That sounds like a different problem. I.e. there are two issues:
>> * threaded interrupts don't work properly (races, ring overflow)
>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>> going to work for RT
>
> I am afraid that's not correct, you can use spin_lock() in threaded
> interrupt handler.

In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
you yourself said above?)

>
>> The first can be fixed by using non-threaded handlers.
>
> The two are somewhat related, if you use a non-threaded handler then
> you are not going to help the RT case.
>
> In general, the unthreaded solution should be used in the last resort.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Another alternative could be to queue the irq if !evtchn->enabled
>>>>>> and
>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>> re-enabled).
>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>
>>>>
>>>> No, I was thinking about having a new structure for deferred
>>>> interrupts.
>>>
>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>> you expand your thinking?
>>
>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>> implemented as a ring but not necessarily as Xen shared ring (if that's
>> what you were referring to).
>
> The underlying question is what happen if you miss an interrupt. Is it
> going to be ok?

This I am not sure about. I thought yes since we are signaling the
process only once.

-boris

> If no, then we have to record everything and can't kill/send an error
> to the user app because it is not its fault.
>
> This means a FIFO would not be a viable. How do you size it? Static
> (i.e pre-defined) size is not going to be possible because you don't
> know how many interrupt you are going to receive before the thread
> handler runs. You can't make it grow dynamically as it make become
> quite big for the same reason.
>
> Discussing with my team, a solution that came up would be to introduce
> one atomic field per event to record the number of event received. I
> will explore that solution tomorrow.
>

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Boris,

On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
> On 2/20/19 3:46 PM, Julien Grall wrote:
>> (+ Andrew and Jan for feedback on the event channel interrupt)
>>
>> Hi Boris,
>>
>> Thank you for the your feedback.
>>
>> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>>> Hi Boris,
>>>>>>
>>>>>> Thank you for your answer.
>>>>>>
>>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>>> started,
>>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>>
>>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>>> be threaded.
>>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>>> the option
>>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>>> that has
>>>>>>>> not RT support).
>>>>>>>>
>>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>>> communicate with
>>>>>>>> xenstore.
>>>>>>>>
>>>>>>>>     From my understanding, this is happening because the interrupt
>>>>>>>> handler is now
>>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>>
>>>>>>>>        Interrupt context            |     Interrupt thread
>>>>>>>>                                     |
>>>>>>>>        receive interrupt port 6     |
>>>>>>>>        clear the evtchn port        |
>>>>>>>>        set IRQF_RUNTHREAD            |
>>>>>>>>        kick interrupt thread        |
>>>>>>>>                                     |    clear IRQF_RUNTHREAD
>>>>>>>>                                     |    call evtchn_interrupt
>>>>>>>>        receive interrupt port 6     |
>>>>>>>>        clear the evtchn port        |
>>>>>>>>        set IRQF_RUNTHREAD           |
>>>>>>>>        kick interrupt thread        |
>>>>>>>>                                     |    disable interrupt port 6
>>>>>>>>                                     |    evtchn->enabled = false
>>>>>>>>                                     |    [....]
>>>>>>>>                                     |
>>>>>>>>                                     |    *** Handling the second
>>>>>>>> interrupt ***
>>>>>>>>                                     |    clear IRQF_RUNTHREAD
>>>>>>>>                                     |    call evtchn_interrupt
>>>>>>>>                                     |    WARN(...)
>>>>>>>>
>>>>>>>> I am not entirely sure how to fix this. I have two solutions in
>>>>>>>> mind:
>>>>>>>>
>>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>>> need to
>>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>>> RT-Linux.
>>>>>>>>
>>>>>>>> 2) Remove the warning
>>>>>>>
>>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>>> warning) we can't use it reliably.
>>>>>>
>>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>>> to contain only one instance of the same event. So if you receive
>>>>>> twice the event, you may overflow the ring.
>>>>>
>>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>>
>>>> I first thought it would be possible to unthread it. However,
>>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>>> so this cannot be used in an interrupt context.
>>>>
>>>> So I think "unthreading" the handler is not an option here.
>>>
>>> That sounds like a different problem. I.e. there are two issues:
>>> * threaded interrupts don't work properly (races, ring overflow)
>>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>>> going to work for RT
>>
>> I am afraid that's not correct, you can use spin_lock() in threaded
>> interrupt handler.
> 
> In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
> you yourself said above?)

In RT-linux, interrupt handlers are threaded by default. So the handler 
will not run in the interrupt context. Hence, it will be safe to call 
spin_lock.

However, if you force the handler to not be threaded (IRQF_NO_THREAD), 
it will run in interrupt context.

>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Another alternative could be to queue the irq if !evtchn->enabled
>>>>>>> and
>>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>>> re-enabled).
>>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>>
>>>>>
>>>>> No, I was thinking about having a new structure for deferred
>>>>> interrupts.
>>>>
>>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>>> you expand your thinking?
>>>
>>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>>> implemented as a ring but not necessarily as Xen shared ring (if that's
>>> what you were referring to).
>>
>> The underlying question is what happen if you miss an interrupt. Is it
>> going to be ok?
> 
> This I am not sure about. I thought yes since we are signaling the
> process only once.

I have CCed Andrew and Jan to see if they can help here.

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Wed, Feb 20, 2019 at 10:03:57PM +0000, Julien Grall wrote:
> Hi Boris,
> 
> On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
> > On 2/20/19 3:46 PM, Julien Grall wrote:
> > > (+ Andrew and Jan for feedback on the event channel interrupt)
> > > 
> > > Hi Boris,
> > > 
> > > Thank you for the your feedback.
> > > 
> > > On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
> > > > On 2/20/19 1:05 PM, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 20/02/2019 17:07, Boris Ostrovsky wrote:
> > > > > > On 2/20/19 9:15 AM, Julien Grall wrote:
> > > > > > > Hi Boris,
> > > > > > > 
> > > > > > > Thank you for your answer.
> > > > > > > 
> > > > > > > On 20/02/2019 00:02, Boris Ostrovsky wrote:
> > > > > > > > On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
> > > > > > > > > Hi all,
> > > > > > > > > 
> > > > > > > > > I have been looking at using Linux RT in Dom0. Once the guest is
> > > > > > > > > started,
> > > > > > > > > the console is ending to have a lot of warning (see trace below).
> > > > > > > > > 
> > > > > > > > > After some investigation, this is because the irq handler will now
> > > > > > > > > be threaded.
> > > > > > > > > I can reproduce the same error with the vanilla Linux when passing
> > > > > > > > > the option
> > > > > > > > > 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
> > > > > > > > > that has
> > > > > > > > > not RT support).
> > > > > > > > > 
> > > > > > > > > FWIW, the interrupt for port 6 is used to for the guest to
> > > > > > > > > communicate with
> > > > > > > > > xenstore.
> > > > > > > > > 
> > > > > > > > >     From my understanding, this is happening because the interrupt
> > > > > > > > > handler is now
> > > > > > > > > run in a thread. So we can have the following happening.
> > > > > > > > > 
> > > > > > > > >        Interrupt context            |     Interrupt thread
> > > > > > > > >                                     |
> > > > > > > > >        receive interrupt port 6     |
> > > > > > > > >        clear the evtchn port        |
> > > > > > > > >        set IRQF_RUNTHREAD            |
> > > > > > > > >        kick interrupt thread        |
> > > > > > > > >                                     |    clear IRQF_RUNTHREAD
> > > > > > > > >                                     |    call evtchn_interrupt
> > > > > > > > >        receive interrupt port 6     |
> > > > > > > > >        clear the evtchn port        |
> > > > > > > > >        set IRQF_RUNTHREAD           |
> > > > > > > > >        kick interrupt thread        |
> > > > > > > > >                                     |    disable interrupt port 6
> > > > > > > > >                                     |    evtchn->enabled = false
> > > > > > > > >                                     |    [....]
> > > > > > > > >                                     |
> > > > > > > > >                                     |    *** Handling the second
> > > > > > > > > interrupt ***
> > > > > > > > >                                     |    clear IRQF_RUNTHREAD
> > > > > > > > >                                     |    call evtchn_interrupt
> > > > > > > > >                                     |    WARN(...)
> > > > > > > > > 
> > > > > > > > > I am not entirely sure how to fix this. I have two solutions in
> > > > > > > > > mind:
> > > > > > > > > 
> > > > > > > > > 1) Prevent the interrupt handler to be threaded. We would also
> > > > > > > > > need to
> > > > > > > > > switch from spin_lock to raw_spin_lock as the former may sleep on
> > > > > > > > > RT-Linux.
> > > > > > > > > 
> > > > > > > > > 2) Remove the warning
> > > > > > > > 
> > > > > > > > I think access to evtchn->enabled is racy so (with or without the
> > > > > > > > warning) we can't use it reliably.
> > > > > > > 
> > > > > > > Thinking about it, it would not be the only issue. The ring is sized
> > > > > > > to contain only one instance of the same event. So if you receive
> > > > > > > twice the event, you may overflow the ring.
> > > > > > 
> > > > > > Hm... That's another argument in favor of "unthreading" the handler.
> > > > > 
> > > > > I first thought it would be possible to unthread it. However,
> > > > > wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
> > > > > so this cannot be used in an interrupt context.
> > > > > 
> > > > > So I think "unthreading" the handler is not an option here.
> > > > 
> > > > That sounds like a different problem. I.e. there are two issues:
> > > > * threaded interrupts don't work properly (races, ring overflow)
> > > > * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
> > > > going to work for RT
> > > 
> > > I am afraid that's not correct, you can use spin_lock() in threaded
> > > interrupt handler.
> > 
> > In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
> > you yourself said above?)
> 
> In RT-linux, interrupt handlers are threaded by default. So the handler will
> not run in the interrupt context. Hence, it will be safe to call spin_lock.
> 
> However, if you force the handler to not be threaded (IRQF_NO_THREAD), it
> will run in interrupt context.
> 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Another alternative could be to queue the irq if !evtchn->enabled
> > > > > > > > and
> > > > > > > > handle it in evtchn_write() (which is where irq is supposed to be
> > > > > > > > re-enabled).
> > > > > > > What do you mean by queue? Is it queueing in the ring?
> > > > > > 
> > > > > > 
> > > > > > No, I was thinking about having a new structure for deferred
> > > > > > interrupts.
> > > > > 
> > > > > Hmmm, I am not entirely sure what would be the structure here. Could
> > > > > you expand your thinking?
> > > > 
> > > > Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
> > > > implemented as a ring but not necessarily as Xen shared ring (if that's
> > > > what you were referring to).
> > > 
> > > The underlying question is what happen if you miss an interrupt. Is it
> > > going to be ok?
> > 
> > This I am not sure about. I thought yes since we are signaling the
> > process only once.
> 
> I have CCed Andrew and Jan to see if they can help here.

FWIW, you can also mask the interrupt while waiting for the thread to
execute the interrupt handler. Ie:

1. Interrupt injected
2. Execute guest event channel callback
3. Scan for pending interrupts
4. Mask interrupt
5. Clear pending field
6. Queue threaded handler
7. Go to 3 until all interrupts are drained
[...]
8. Execute interrupt handler in thread
9. Unmask interrupt

That should prevent you from stacking interrupts?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Wed, Feb 20, 2019 at 10:03:57PM +0000, Julien Grall wrote:
> Hi Boris,
> 
> On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
> > On 2/20/19 3:46 PM, Julien Grall wrote:
> > > (+ Andrew and Jan for feedback on the event channel interrupt)
> > > 
> > > Hi Boris,
> > > 
> > > Thank you for the your feedback.
> > > 
> > > On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
> > > > On 2/20/19 1:05 PM, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 20/02/2019 17:07, Boris Ostrovsky wrote:
> > > > > > On 2/20/19 9:15 AM, Julien Grall wrote:
> > > > > > > Hi Boris,
> > > > > > > 
> > > > > > > Thank you for your answer.
> > > > > > > 
> > > > > > > On 20/02/2019 00:02, Boris Ostrovsky wrote:
> > > > > > > > On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
> > > > > > > > > Hi all,
> > > > > > > > > 
> > > > > > > > > I have been looking at using Linux RT in Dom0. Once the guest is
> > > > > > > > > started,
> > > > > > > > > the console is ending to have a lot of warning (see trace below).
> > > > > > > > > 
> > > > > > > > > After some investigation, this is because the irq handler will now
> > > > > > > > > be threaded.
> > > > > > > > > I can reproduce the same error with the vanilla Linux when passing
> > > > > > > > > the option
> > > > > > > > > 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
> > > > > > > > > that has
> > > > > > > > > not RT support).
> > > > > > > > > 
> > > > > > > > > FWIW, the interrupt for port 6 is used to for the guest to
> > > > > > > > > communicate with
> > > > > > > > > xenstore.
> > > > > > > > > 
> > > > > > > > >     From my understanding, this is happening because the interrupt
> > > > > > > > > handler is now
> > > > > > > > > run in a thread. So we can have the following happening.
> > > > > > > > > 
> > > > > > > > >        Interrupt context            |     Interrupt thread
> > > > > > > > >                                     |
> > > > > > > > >        receive interrupt port 6     |
> > > > > > > > >        clear the evtchn port        |
> > > > > > > > >        set IRQF_RUNTHREAD            |
> > > > > > > > >        kick interrupt thread        |
> > > > > > > > >                                     |    clear IRQF_RUNTHREAD
> > > > > > > > >                                     |    call evtchn_interrupt
> > > > > > > > >        receive interrupt port 6     |
> > > > > > > > >        clear the evtchn port        |
> > > > > > > > >        set IRQF_RUNTHREAD           |
> > > > > > > > >        kick interrupt thread        |
> > > > > > > > >                                     |    disable interrupt port 6
> > > > > > > > >                                     |    evtchn->enabled = false
> > > > > > > > >                                     |    [....]
> > > > > > > > >                                     |
> > > > > > > > >                                     |    *** Handling the second
> > > > > > > > > interrupt ***
> > > > > > > > >                                     |    clear IRQF_RUNTHREAD
> > > > > > > > >                                     |    call evtchn_interrupt
> > > > > > > > >                                     |    WARN(...)
> > > > > > > > > 
> > > > > > > > > I am not entirely sure how to fix this. I have two solutions in
> > > > > > > > > mind:
> > > > > > > > > 
> > > > > > > > > 1) Prevent the interrupt handler to be threaded. We would also
> > > > > > > > > need to
> > > > > > > > > switch from spin_lock to raw_spin_lock as the former may sleep on
> > > > > > > > > RT-Linux.
> > > > > > > > > 
> > > > > > > > > 2) Remove the warning
> > > > > > > > 
> > > > > > > > I think access to evtchn->enabled is racy so (with or without the
> > > > > > > > warning) we can't use it reliably.
> > > > > > > 
> > > > > > > Thinking about it, it would not be the only issue. The ring is sized
> > > > > > > to contain only one instance of the same event. So if you receive
> > > > > > > twice the event, you may overflow the ring.
> > > > > > 
> > > > > > Hm... That's another argument in favor of "unthreading" the handler.
> > > > > 
> > > > > I first thought it would be possible to unthread it. However,
> > > > > wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
> > > > > so this cannot be used in an interrupt context.
> > > > > 
> > > > > So I think "unthreading" the handler is not an option here.
> > > > 
> > > > That sounds like a different problem. I.e. there are two issues:
> > > > * threaded interrupts don't work properly (races, ring overflow)
> > > > * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
> > > > going to work for RT
> > > 
> > > I am afraid that's not correct, you can use spin_lock() in threaded
> > > interrupt handler.
> > 
> > In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
> > you yourself said above?)
> 
> In RT-linux, interrupt handlers are threaded by default. So the handler will
> not run in the interrupt context. Hence, it will be safe to call spin_lock.
> 
> However, if you force the handler to not be threaded (IRQF_NO_THREAD), it
> will run in interrupt context.
> 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Another alternative could be to queue the irq if !evtchn->enabled
> > > > > > > > and
> > > > > > > > handle it in evtchn_write() (which is where irq is supposed to be
> > > > > > > > re-enabled).
> > > > > > > What do you mean by queue? Is it queueing in the ring?
> > > > > > 
> > > > > > 
> > > > > > No, I was thinking about having a new structure for deferred
> > > > > > interrupts.
> > > > > 
> > > > > Hmmm, I am not entirely sure what would be the structure here. Could
> > > > > you expand your thinking?
> > > > 
> > > > Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
> > > > implemented as a ring but not necessarily as Xen shared ring (if that's
> > > > what you were referring to).
> > > 
> > > The underlying question is what happen if you miss an interrupt. Is it
> > > going to be ok?
> > 
> > This I am not sure about. I thought yes since we are signaling the
> > process only once.
> 
> I have CCed Andrew and Jan to see if they can help here.

FWIW, you can also mask the interrupt while waiting for the thread to
execute the interrupt handler. Ie:

1. Interrupt injected
2. Execute guest event channel callback
3. Scan for pending interrupts
4. Mask interrupt
5. Clear pending field
6. Queue threaded handler
7. Go to 3 until all interrupts are drained
[...]
8. Execute interrupt handler in thread
9. Unmask interrupt

That should prevent you from stacking interrupts?

Roger.

Re: xen/evtchn and forced threaded irq

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 693 bytes --]

Hi Roger,

On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:

> FWIW, you can also mask the interrupt while waiting for the thread to
> execute the interrupt handler. Ie:
>

Thank you for providing steps, however where would the masking be done? By
the irqchip or a custom solution?


> 1. Interrupt injected
> 2. Execute guest event channel callback
> 3. Scan for pending interrupts
> 4. Mask interrupt
> 5. Clear pending field
> 6. Queue threaded handler
> 7. Go to 3 until all interrupts are drained
> [...]
> 8. Execute interrupt handler in thread
> 9. Unmask interrupt
>
> That should prevent you from stacking interrupts?
>
> Roger.
>

[-- Attachment #1.2: Type: text/html, Size: 1124 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Juergen Gross]

On 21/02/2019 09:38, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com
> <mailto:roger.pau@citrix.com>> wrote:
> 
>     FWIW, you can also mask the interrupt while waiting for the thread to
>     execute the interrupt handler. Ie:
> 
> 
> Thank you for providing steps, however where would the masking be done?
> By the irqchip or a custom solution?

I'd go the irqchip route. This would be the least intrusive change
without the need for handling RT in a special way.


Juergen

Re: xen/evtchn and forced threaded irq

[Juergen Gross]

On 21/02/2019 09:38, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com
> <mailto:roger.pau@citrix.com>> wrote:
> 
>     FWIW, you can also mask the interrupt while waiting for the thread to
>     execute the interrupt handler. Ie:
> 
> 
> Thank you for providing steps, however where would the masking be done?
> By the irqchip or a custom solution?

I'd go the irqchip route. This would be the least intrusive change
without the need for handling RT in a special way.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Thu, Feb 21, 2019 at 08:38:39AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
> 
> > FWIW, you can also mask the interrupt while waiting for the thread to
> > execute the interrupt handler. Ie:
> >
> 
> Thank you for providing steps, however where would the masking be done? By
> the irqchip or a custom solution?

I'm not familiar with the irqchip infrastructure in Linux, what I
proposed below is what FreeBSD does when running interrupt handlers in
deferred threads IIRC.

If irqchip has a specific handler to dispatch to a thread, then that's
the place where the masking should happen. Likely, the unmasking
should be done by the irq handling infrastructure after the thread
executing the interrupt handler has finished.

Isn't there a similar way to handle interrupts in threads for Linux?

> 
> > 1. Interrupt injected
> > 2. Execute guest event channel callback
> > 3. Scan for pending interrupts
> > 4. Mask interrupt
> > 5. Clear pending field
> > 6. Queue threaded handler
> > 7. Go to 3 until all interrupts are drained
> > [...]
> > 8. Execute interrupt handler in thread
> > 9. Unmask interrupt
> >
> > That should prevent you from stacking interrupts?
> >
> > Roger.
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Thu, Feb 21, 2019 at 08:38:39AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
> 
> > FWIW, you can also mask the interrupt while waiting for the thread to
> > execute the interrupt handler. Ie:
> >
> 
> Thank you for providing steps, however where would the masking be done? By
> the irqchip or a custom solution?

I'm not familiar with the irqchip infrastructure in Linux, what I
proposed below is what FreeBSD does when running interrupt handlers in
deferred threads IIRC.

If irqchip has a specific handler to dispatch to a thread, then that's
the place where the masking should happen. Likely, the unmasking
should be done by the irq handling infrastructure after the thread
executing the interrupt handler has finished.

Isn't there a similar way to handle interrupts in threads for Linux?

> 
> > 1. Interrupt injected
> > 2. Execute guest event channel callback
> > 3. Scan for pending interrupts
> > 4. Mask interrupt
> > 5. Clear pending field
> > 6. Queue threaded handler
> > 7. Go to 3 until all interrupts are drained
> > [...]
> > 8. Execute interrupt handler in thread
> > 9. Unmask interrupt
> >
> > That should prevent you from stacking interrupts?
> >
> > Roger.
> >

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi Roger,

On 21/02/2019 09:14, Roger Pau Monné wrote:
> On Thu, Feb 21, 2019 at 08:38:39AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
>>
>>> FWIW, you can also mask the interrupt while waiting for the thread to
>>> execute the interrupt handler. Ie:
>>>
>>
>> Thank you for providing steps, however where would the masking be done? By
>> the irqchip or a custom solution?
>
> I'm not familiar with the irqchip infrastructure in Linux, what I
> proposed below is what FreeBSD does when running interrupt handlers in
> deferred threads IIRC.
>
> If irqchip has a specific handler to dispatch to a thread, then that's
> the place where the masking should happen. Likely, the unmasking
> should be done by the irq handling infrastructure after the thread
> executing the interrupt handler has finished.
>
> Isn't there a similar way to handle interrupts in threads for Linux?

Linux has a flag (IRQF_ONESHOT) to mask interrupt until the interrupt
handler has been run. It is set for all interrupts handler that have
been forced to be threaded.

However, it looks like the flag is been ignored by the irqchip handler
we use (handle_edge_irq). Doing a bit of digging, IRQF_ONESHOT use to be
handled in handle_edge_irq until the following commit from 2009:

commit 4dbc9ca219b0f294332e734528f7b82211700170
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Aug 27 09:38:49 2009 +0200

     genirq: Do not mask oneshot edge type interrupts

     Masking oneshot edge type interrupts is wrong as we might lose an
     interrupt which is issued when the threaded handler is handling the
     device. We can keep the irq unmasked safely as with edge type
     interrupts there is no danger of interrupt floods. If the threaded
     handler has not yet finished then IRQTF_RUNTHREAD is set which will
     keep the handler thread active.

     Debugged and verified in preempt-rt.

     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Furthermore, it is pretty clear from the comment on top of
handle_edge_irq that the same interrupt can come-up before the first one
is one is handled by the associated event handler.

I am still trying to figure out whether the issue lies in the evtchn
driver or the Xen irqchip (events_base.c). I will have a closer look and
come back with updates here.

Cheers,

--
Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Roger,

On 21/02/2019 09:14, Roger Pau Monné wrote:
> On Thu, Feb 21, 2019 at 08:38:39AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
>>
>>> FWIW, you can also mask the interrupt while waiting for the thread to
>>> execute the interrupt handler. Ie:
>>>
>>
>> Thank you for providing steps, however where would the masking be done? By
>> the irqchip or a custom solution?
>
> I'm not familiar with the irqchip infrastructure in Linux, what I
> proposed below is what FreeBSD does when running interrupt handlers in
> deferred threads IIRC.
>
> If irqchip has a specific handler to dispatch to a thread, then that's
> the place where the masking should happen. Likely, the unmasking
> should be done by the irq handling infrastructure after the thread
> executing the interrupt handler has finished.
>
> Isn't there a similar way to handle interrupts in threads for Linux?

Linux has a flag (IRQF_ONESHOT) to mask interrupt until the interrupt
handler has been run. It is set for all interrupts handler that have
been forced to be threaded.

However, it looks like the flag is been ignored by the irqchip handler
we use (handle_edge_irq). Doing a bit of digging, IRQF_ONESHOT use to be
handled in handle_edge_irq until the following commit from 2009:

commit 4dbc9ca219b0f294332e734528f7b82211700170
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Aug 27 09:38:49 2009 +0200

     genirq: Do not mask oneshot edge type interrupts

     Masking oneshot edge type interrupts is wrong as we might lose an
     interrupt which is issued when the threaded handler is handling the
     device. We can keep the irq unmasked safely as with edge type
     interrupts there is no danger of interrupt floods. If the threaded
     handler has not yet finished then IRQTF_RUNTHREAD is set which will
     keep the handler thread active.

     Debugged and verified in preempt-rt.

     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Furthermore, it is pretty clear from the comment on top of
handle_edge_irq that the same interrupt can come-up before the first one
is one is handled by the associated event handler.

I am still trying to figure out whether the issue lies in the evtchn
driver or the Xen irqchip (events_base.c). I will have a closer look and
come back with updates here.

Cheers,

--
Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Thu, 21 Feb 2019, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
>       FWIW, you can also mask the interrupt while waiting for the thread to
>       execute the interrupt handler. Ie:
> 
> 
> Thank you for providing steps, however where would the masking be done? By the irqchip or a custom solution?
> 
> 
>       1. Interrupt injected
>       2. Execute guest event channel callback
>       3. Scan for pending interrupts
>       4. Mask interrupt
>       5. Clear pending field
>       6. Queue threaded handler
>       7. Go to 3 until all interrupts are drained
>       [...]
>       8. Execute interrupt handler in thread
>       9. Unmask interrupt
> 
>       That should prevent you from stacking interrupts?

Sorry for coming late to the thread, and thanks Julien for pointing it
out to me. I am afraid I was the one to break the flow back in 2011 with
the following commit:

  7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Oops :-)


Xen event channels have their own workflow; the one Roger wrote above.
They used to be handled using handle_fasteoi_irq until 7e186bdd0098,
then I switched (almost) all of them to handle_edge_irq.

Looking closely at irq handling again, it doesn't look like we can do
what we need with handle_edge_irq today: we can't mask the event channel
before clearing it. But we can do that if we go back to using
handle_fasteoi_irq.

In fact, I managed to verify that LinuxRT works fine as dom0 with the
attached dynamic.patch that switches back xen_dynamic_chip IRQs to
handle_fasteoi_irq.


From the rest of this thread, it looks like the issue might appear with
PIRQs as well. Thus, I wrote a second patch pirqs.patch to switch back
to handle_fasteoi_irq PIRQs as well. However, Xen on ARM does not use
PIRQs so I couldn't test it at all. I would appreciate if Boris/Juegen
tested it. Let me know what you want me to do with the second patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=dynamic.patch, Size: 2643 bytes --]

From ce26c371a8ff7b49c98a3b8c7b57199154cbca59 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <sstabellini@kernel.org>
Date: Mon, 27 Apr 2020 16:15:26 -0700
Subject: [PATCH] xen: use handle_fasteoi_irq to handle xen events

When handling Xen events, we need to make sure the following sequence is
followed:

- mask event
- handle event and clear event (the order does not matter)
- unmask event

It is not possible to implement this flow with handle_edge_irq, so
switch back to handle_fasteoi_irq. Please note that Xen event irqs are
ONESHOT. Also note that handle_fasteoi_irq was in-use before the
following commit, that is partially reverted by this patch:

7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

PIRQ handling is left unchanged.

This patch fixes a domU hang observed when using LinuxRT as dom0 kernel.

Link: https://lore.kernel.org/lkml/5e256d9a-572c-e01e-7706-407f99245b00@arm.com/
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 drivers/xen/events/events_base.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 499eff7d3f65..5f9b8104dbcf 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -845,7 +845,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 			goto out;
 
 		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_edge_irq, "event");
+					      handle_fasteoi_irq, "event");
 
 		ret = xen_irq_info_evtchn_setup(irq, evtchn);
 		if (ret < 0) {
@@ -978,7 +978,7 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 						      handle_percpu_irq, "virq");
 		else
 			irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-						      handle_edge_irq, "virq");
+						      handle_fasteoi_irq, "virq");
 
 		bind_virq.virq = virq;
 		bind_virq.vcpu = xen_vcpu_nr(cpu);
@@ -1377,12 +1377,6 @@ static void ack_dynirq(struct irq_data *data)
 		clear_evtchn(evtchn);
 }
 
-static void mask_ack_dynirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	ack_dynirq(data);
-}
-
 static int retrigger_dynirq(struct irq_data *data)
 {
 	unsigned int evtchn = evtchn_from_irq(data->irq);
@@ -1585,8 +1579,7 @@ static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= ack_dynirq,
-	.irq_mask_ack		= mask_ack_dynirq,
+	.irq_eoi		= ack_dynirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 	.irq_retrigger		= retrigger_dynirq,
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=pirqs.patch, Size: 2492 bytes --]

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 5f9b8104dbcf..57a29c94fefc 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -498,12 +498,6 @@ static void eoi_pirq(struct irq_data *data)
 	}
 }
 
-static void mask_ack_pirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	eoi_pirq(data);
-}
-
 static unsigned int __startup_pirq(unsigned int irq)
 {
 	struct evtchn_bind_pirq bind_pirq;
@@ -684,13 +678,9 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	}
 
 	pirq_query_unmask(irq);
-	/* We try to use the handler with the appropriate semantic for the
-	 * type of interrupt: if the interrupt is an edge triggered
-	 * interrupt we use handle_edge_irq.
-	 *
-	 * On the other hand if the interrupt is level triggered we use
-	 * handle_fasteoi_irq like the native code does for this kind of
-	 * interrupts.
+	/* We use handle_fasteoi_irq for PIRQs because we want to keep
+	 * the evtchn masked while handling and clearing the event.
+	 * Unmasking the evtchn should only happen after clearing it.
 	 *
 	 * Depending on the Xen version, pirq_needs_eoi might return true
 	 * not only for level triggered interrupts but for edge triggered
@@ -699,12 +689,8 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * hasn't received an eoi yet. Therefore using the fasteoi handler
 	 * is the right choice either way.
 	 */
-	if (shareable)
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_fasteoi_irq, name);
-	else
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_edge_irq, name);
+	irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+			handle_fasteoi_irq, name);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -739,7 +725,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		goto out;
 
 	for (i = 0; i < nvec; i++) {
-		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
+		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_fasteoi_irq, name);
 
 		ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
 					      i == 0 ? 0 : PIRQ_MSI_GROUP);
@@ -1596,9 +1582,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= eoi_pirq,
 	.irq_eoi		= eoi_pirq,
-	.irq_mask_ack		= mask_ack_pirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 
-- 
2.17.1

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Thu, 21 Feb 2019, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
>       FWIW, you can also mask the interrupt while waiting for the thread to
>       execute the interrupt handler. Ie:
> 
> 
> Thank you for providing steps, however where would the masking be done? By the irqchip or a custom solution?
> 
> 
>       1. Interrupt injected
>       2. Execute guest event channel callback
>       3. Scan for pending interrupts
>       4. Mask interrupt
>       5. Clear pending field
>       6. Queue threaded handler
>       7. Go to 3 until all interrupts are drained
>       [...]
>       8. Execute interrupt handler in thread
>       9. Unmask interrupt
> 
>       That should prevent you from stacking interrupts?

Sorry for coming late to the thread, and thanks Julien for pointing it
out to me. I am afraid I was the one to break the flow back in 2011 with
the following commit:

  7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Oops :-)


Xen event channels have their own workflow; the one Roger wrote above.
They used to be handled using handle_fasteoi_irq until 7e186bdd0098,
then I switched (almost) all of them to handle_edge_irq.

Looking closely at irq handling again, it doesn't look like we can do
what we need with handle_edge_irq today: we can't mask the event channel
before clearing it. But we can do that if we go back to using
handle_fasteoi_irq.

In fact, I managed to verify that LinuxRT works fine as dom0 with the
attached dynamic.patch that switches back xen_dynamic_chip IRQs to
handle_fasteoi_irq.


From the rest of this thread, it looks like the issue might appear with
PIRQs as well. Thus, I wrote a second patch pirqs.patch to switch back
to handle_fasteoi_irq PIRQs as well. However, Xen on ARM does not use
PIRQs so I couldn't test it at all. I would appreciate if Boris/Juegen
tested it. Let me know what you want me to do with the second patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=dynamic.patch, Size: 2643 bytes --]

From ce26c371a8ff7b49c98a3b8c7b57199154cbca59 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <sstabellini@kernel.org>
Date: Mon, 27 Apr 2020 16:15:26 -0700
Subject: [PATCH] xen: use handle_fasteoi_irq to handle xen events

When handling Xen events, we need to make sure the following sequence is
followed:

- mask event
- handle event and clear event (the order does not matter)
- unmask event

It is not possible to implement this flow with handle_edge_irq, so
switch back to handle_fasteoi_irq. Please note that Xen event irqs are
ONESHOT. Also note that handle_fasteoi_irq was in-use before the
following commit, that is partially reverted by this patch:

7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

PIRQ handling is left unchanged.

This patch fixes a domU hang observed when using LinuxRT as dom0 kernel.

Link: https://lore.kernel.org/lkml/5e256d9a-572c-e01e-7706-407f99245b00@arm.com/
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 drivers/xen/events/events_base.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 499eff7d3f65..5f9b8104dbcf 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -845,7 +845,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 			goto out;
 
 		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_edge_irq, "event");
+					      handle_fasteoi_irq, "event");
 
 		ret = xen_irq_info_evtchn_setup(irq, evtchn);
 		if (ret < 0) {
@@ -978,7 +978,7 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 						      handle_percpu_irq, "virq");
 		else
 			irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-						      handle_edge_irq, "virq");
+						      handle_fasteoi_irq, "virq");
 
 		bind_virq.virq = virq;
 		bind_virq.vcpu = xen_vcpu_nr(cpu);
@@ -1377,12 +1377,6 @@ static void ack_dynirq(struct irq_data *data)
 		clear_evtchn(evtchn);
 }
 
-static void mask_ack_dynirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	ack_dynirq(data);
-}
-
 static int retrigger_dynirq(struct irq_data *data)
 {
 	unsigned int evtchn = evtchn_from_irq(data->irq);
@@ -1585,8 +1579,7 @@ static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= ack_dynirq,
-	.irq_mask_ack		= mask_ack_dynirq,
+	.irq_eoi		= ack_dynirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 	.irq_retrigger		= retrigger_dynirq,
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=pirqs.patch, Size: 2492 bytes --]

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 5f9b8104dbcf..57a29c94fefc 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -498,12 +498,6 @@ static void eoi_pirq(struct irq_data *data)
 	}
 }
 
-static void mask_ack_pirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	eoi_pirq(data);
-}
-
 static unsigned int __startup_pirq(unsigned int irq)
 {
 	struct evtchn_bind_pirq bind_pirq;
@@ -684,13 +678,9 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	}
 
 	pirq_query_unmask(irq);
-	/* We try to use the handler with the appropriate semantic for the
-	 * type of interrupt: if the interrupt is an edge triggered
-	 * interrupt we use handle_edge_irq.
-	 *
-	 * On the other hand if the interrupt is level triggered we use
-	 * handle_fasteoi_irq like the native code does for this kind of
-	 * interrupts.
+	/* We use handle_fasteoi_irq for PIRQs because we want to keep
+	 * the evtchn masked while handling and clearing the event.
+	 * Unmasking the evtchn should only happen after clearing it.
 	 *
 	 * Depending on the Xen version, pirq_needs_eoi might return true
 	 * not only for level triggered interrupts but for edge triggered
@@ -699,12 +689,8 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * hasn't received an eoi yet. Therefore using the fasteoi handler
 	 * is the right choice either way.
 	 */
-	if (shareable)
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_fasteoi_irq, name);
-	else
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_edge_irq, name);
+	irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+			handle_fasteoi_irq, name);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -739,7 +725,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		goto out;
 
 	for (i = 0; i < nvec; i++) {
-		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
+		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_fasteoi_irq, name);
 
 		ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
 					      i == 0 ? 0 : PIRQ_MSI_GROUP);
@@ -1596,9 +1582,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= eoi_pirq,
 	.irq_eoi		= eoi_pirq,
-	.irq_mask_ack		= mask_ack_pirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 
-- 
2.17.1

Re: xen/evtchn and forced threaded irq

[Jan Beulich]

>>> On 20.02.19 at 23:03, <julien.grall@arm.com> wrote:
> On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
>> On 2/20/19 3:46 PM, Julien Grall wrote:
>>> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>>>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>>>> implemented as a ring but not necessarily as Xen shared ring (if that's
>>>> what you were referring to).

I'm sort of lost here with the mixed references to "interrupts" and
"events". Interrupts can be shared (and have a per-instance
(irq,data) tuple), but there should be at most one interrupt
underlying the event channel systems, shouldn't there? Event
channels otoh can't be shared, and hence there's only one
"data" item to be associated with each channel, at which point a
plain counter ought to do.

>>> The underlying question is what happen if you miss an interrupt. Is it
>>> going to be ok?
>> 
>> This I am not sure about. I thought yes since we are signaling the
>> process only once.
> 
> I have CCed Andrew and Jan to see if they can help here.

What meaning of "miss" do you use here? As long as is only means
delay (i.e. miss an instance, but get notified as soon as feasible
even if there is not further event coming from the source) - that
would be okay, I think. But if you truly mean "miss" (i.e. event lost
altogether, with silence resulting if there's no further event), then
no, this would not be tolerable.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Jan Beulich]

>>> On 20.02.19 at 23:03, <julien.grall@arm.com> wrote:
> On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
>> On 2/20/19 3:46 PM, Julien Grall wrote:
>>> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>>>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>>>> implemented as a ring but not necessarily as Xen shared ring (if that's
>>>> what you were referring to).

I'm sort of lost here with the mixed references to "interrupts" and
"events". Interrupts can be shared (and have a per-instance
(irq,data) tuple), but there should be at most one interrupt
underlying the event channel systems, shouldn't there? Event
channels otoh can't be shared, and hence there's only one
"data" item to be associated with each channel, at which point a
plain counter ought to do.

>>> The underlying question is what happen if you miss an interrupt. Is it
>>> going to be ok?
>> 
>> This I am not sure about. I thought yes since we are signaling the
>> process only once.
> 
> I have CCed Andrew and Jan to see if they can help here.

What meaning of "miss" do you use here? As long as is only means
delay (i.e. miss an instance, but get notified as soon as feasible
even if there is not further event coming from the source) - that
would be okay, I think. But if you truly mean "miss" (i.e. event lost
altogether, with silence resulting if there's no further event), then
no, this would not be tolerable.

Jan

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Boris,

On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
> On 2/20/19 3:46 PM, Julien Grall wrote:
>> (+ Andrew and Jan for feedback on the event channel interrupt)
>>
>> Hi Boris,
>>
>> Thank you for the your feedback.
>>
>> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>>> Hi Boris,
>>>>>>
>>>>>> Thank you for your answer.
>>>>>>
>>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>>> started,
>>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>>
>>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>>> be threaded.
>>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>>> the option
>>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>>> that has
>>>>>>>> not RT support).
>>>>>>>>
>>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>>> communicate with
>>>>>>>> xenstore.
>>>>>>>>
>>>>>>>>     From my understanding, this is happening because the interrupt
>>>>>>>> handler is now
>>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>>
>>>>>>>>        Interrupt context            |     Interrupt thread
>>>>>>>>                                     |
>>>>>>>>        receive interrupt port 6     |
>>>>>>>>        clear the evtchn port        |
>>>>>>>>        set IRQF_RUNTHREAD            |
>>>>>>>>        kick interrupt thread        |
>>>>>>>>                                     |    clear IRQF_RUNTHREAD
>>>>>>>>                                     |    call evtchn_interrupt
>>>>>>>>        receive interrupt port 6     |
>>>>>>>>        clear the evtchn port        |
>>>>>>>>        set IRQF_RUNTHREAD           |
>>>>>>>>        kick interrupt thread        |
>>>>>>>>                                     |    disable interrupt port 6
>>>>>>>>                                     |    evtchn->enabled = false
>>>>>>>>                                     |    [....]
>>>>>>>>                                     |
>>>>>>>>                                     |    *** Handling the second
>>>>>>>> interrupt ***
>>>>>>>>                                     |    clear IRQF_RUNTHREAD
>>>>>>>>                                     |    call evtchn_interrupt
>>>>>>>>                                     |    WARN(...)
>>>>>>>>
>>>>>>>> I am not entirely sure how to fix this. I have two solutions in
>>>>>>>> mind:
>>>>>>>>
>>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>>> need to
>>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>>> RT-Linux.
>>>>>>>>
>>>>>>>> 2) Remove the warning
>>>>>>>
>>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>>> warning) we can't use it reliably.
>>>>>>
>>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>>> to contain only one instance of the same event. So if you receive
>>>>>> twice the event, you may overflow the ring.
>>>>>
>>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>>
>>>> I first thought it would be possible to unthread it. However,
>>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>>> so this cannot be used in an interrupt context.
>>>>
>>>> So I think "unthreading" the handler is not an option here.
>>>
>>> That sounds like a different problem. I.e. there are two issues:
>>> * threaded interrupts don't work properly (races, ring overflow)
>>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>>> going to work for RT
>>
>> I am afraid that's not correct, you can use spin_lock() in threaded
>> interrupt handler.
> 
> In non-RT handler -- yes, but not in an RT one (in fact, isn't this what
> you yourself said above?)

In RT-linux, interrupt handlers are threaded by default. So the handler 
will not run in the interrupt context. Hence, it will be safe to call 
spin_lock.

However, if you force the handler to not be threaded (IRQF_NO_THREAD), 
it will run in interrupt context.

>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Another alternative could be to queue the irq if !evtchn->enabled
>>>>>>> and
>>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>>> re-enabled).
>>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>>
>>>>>
>>>>> No, I was thinking about having a new structure for deferred
>>>>> interrupts.
>>>>
>>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>>> you expand your thinking?
>>>
>>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>>> implemented as a ring but not necessarily as Xen shared ring (if that's
>>> what you were referring to).
>>
>> The underlying question is what happen if you miss an interrupt. Is it
>> going to be ok?
> 
> This I am not sure about. I thought yes since we are signaling the
> process only once.

I have CCed Andrew and Jan to see if they can help here.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/20/19 10:46 PM, Julien Grall wrote:
> (+ Andrew and Jan for feedback on the event channel interrupt)
>
> Hi Boris,
>
> Thank you for the your feedback.
>
> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>> Hi Boris,
>>>>>
>>>>> Thank you for your answer.
>>>>>
>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>> started,
>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>
>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>> be threaded.
>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>> the option
>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>> that has
>>>>>>> not RT support).
>>>>>>>
>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>> communicate with
>>>>>>> xenstore.
>>>>>>>
>>>>>>>    From my understanding, this is happening because the interrupt
>>>>>>> handler is now
>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>
>>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>>                                    |
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    disable interrupt port 6
>>>>>>>                                    | evtchn->enabled = false
>>>>>>>                                    |    [....]
>>>>>>>                                    |
>>>>>>>                                    |    *** Handling the second
>>>>>>> interrupt ***
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>                                    |    WARN(...)
>>>>>>>
>>>>>>> I am not entirely sure how to fix this. I have two solutions in 
>>>>>>> mind:
>>>>>>>
>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>> need to
>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>> RT-Linux.
>>>>>>>
>>>>>>> 2) Remove the warning
>>>>>>
>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>> warning) we can't use it reliably.
>>>>>
>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>> to contain only one instance of the same event. So if you receive
>>>>> twice the event, you may overflow the ring.
>>>>
>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>
>>> I first thought it would be possible to unthread it. However,
>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>> so this cannot be used in an interrupt context.
>>>
>>> So I think "unthreading" the handler is not an option here.
>>
>> That sounds like a different problem. I.e. there are two issues:
>> * threaded interrupts don't work properly (races, ring overflow)
>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>> going to work for RT
>
> I am afraid that's not correct, you can use spin_lock() in threaded 
> interrupt handler.
>
>> The first can be fixed by using non-threaded handlers.
>
> The two are somewhat related, if you use a non-threaded handler then 
> you are not going to help the RT case.
>
> In general, the unthreaded solution should be used in the last resort.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Another alternative could be to queue the irq if !evtchn->enabled 
>>>>>> and
>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>> re-enabled).
>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>
>>>>
>>>> No, I was thinking about having a new structure for deferred 
>>>> interrupts.
>>>
>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>> you expand your thinking?
>>
>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>> implemented as a ring but not necessarily as Xen shared ring (if that's
>> what you were referring to).
>
> The underlying question is what happen if you miss an interrupt. Is it 
> going to be ok? If no, then we have to record everything and can't 
> kill/send an error to the user app because it is not its fault.
>
> This means a FIFO would not be a viable. How do you size it? Static 
> (i.e pre-defined) size is not going to be possible because you don't 
> know how many interrupt you are going to receive before the thread 
> handler runs. You can't make it grow dynamically as it make become 
> quite big for the same reason.
>
> Discussing with my team, a solution that came up would be to introduce 
> one atomic field per event to record the number of event received. I 
> will explore that solution tomorrow.
How will this help if events have some payload?
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/20/19 10:46 PM, Julien Grall wrote:
> (+ Andrew and Jan for feedback on the event channel interrupt)
>
> Hi Boris,
>
> Thank you for the your feedback.
>
> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
>> On 2/20/19 1:05 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>>> Hi Boris,
>>>>>
>>>>> Thank you for your answer.
>>>>>
>>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>>> started,
>>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>>
>>>>>>> After some investigation, this is because the irq handler will now
>>>>>>> be threaded.
>>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>>> the option
>>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>>> that has
>>>>>>> not RT support).
>>>>>>>
>>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>>> communicate with
>>>>>>> xenstore.
>>>>>>>
>>>>>>>    From my understanding, this is happening because the interrupt
>>>>>>> handler is now
>>>>>>> run in a thread. So we can have the following happening.
>>>>>>>
>>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>>                                    |
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>       receive interrupt port 6     |
>>>>>>>       clear the evtchn port        |
>>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>>       kick interrupt thread        |
>>>>>>>                                    |    disable interrupt port 6
>>>>>>>                                    | evtchn->enabled = false
>>>>>>>                                    |    [....]
>>>>>>>                                    |
>>>>>>>                                    |    *** Handling the second
>>>>>>> interrupt ***
>>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>>                                    |    call evtchn_interrupt
>>>>>>>                                    |    WARN(...)
>>>>>>>
>>>>>>> I am not entirely sure how to fix this. I have two solutions in 
>>>>>>> mind:
>>>>>>>
>>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>>> need to
>>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>>> RT-Linux.
>>>>>>>
>>>>>>> 2) Remove the warning
>>>>>>
>>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>>> warning) we can't use it reliably.
>>>>>
>>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>>> to contain only one instance of the same event. So if you receive
>>>>> twice the event, you may overflow the ring.
>>>>
>>>> Hm... That's another argument in favor of "unthreading" the handler.
>>>
>>> I first thought it would be possible to unthread it. However,
>>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>>> so this cannot be used in an interrupt context.
>>>
>>> So I think "unthreading" the handler is not an option here.
>>
>> That sounds like a different problem. I.e. there are two issues:
>> * threaded interrupts don't work properly (races, ring overflow)
>> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
>> going to work for RT
>
> I am afraid that's not correct, you can use spin_lock() in threaded 
> interrupt handler.
>
>> The first can be fixed by using non-threaded handlers.
>
> The two are somewhat related, if you use a non-threaded handler then 
> you are not going to help the RT case.
>
> In general, the unthreaded solution should be used in the last resort.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Another alternative could be to queue the irq if !evtchn->enabled 
>>>>>> and
>>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>>> re-enabled).
>>>>> What do you mean by queue? Is it queueing in the ring?
>>>>
>>>>
>>>> No, I was thinking about having a new structure for deferred 
>>>> interrupts.
>>>
>>> Hmmm, I am not entirely sure what would be the structure here. Could
>>> you expand your thinking?
>>
>> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
>> implemented as a ring but not necessarily as Xen shared ring (if that's
>> what you were referring to).
>
> The underlying question is what happen if you miss an interrupt. Is it 
> going to be ok? If no, then we have to record everything and can't 
> kill/send an error to the user app because it is not its fault.
>
> This means a FIFO would not be a viable. How do you size it? Static 
> (i.e pre-defined) size is not going to be possible because you don't 
> know how many interrupt you are going to receive before the thread 
> handler runs. You can't make it grow dynamically as it make become 
> quite big for the same reason.
>
> Discussing with my team, a solution that came up would be to introduce 
> one atomic field per event to record the number of event received. I 
> will explore that solution tomorrow.
How will this help if events have some payload?
>
> Cheers,
>

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> On 2/20/19 10:46 PM, Julien Grall wrote:
>> Discussing with my team, a solution that came up would be to introduce one 
>> atomic field per event to record the number of event received. I will explore 
>> that solution tomorrow.
> How will this help if events have some payload?

What payload? The event channel does not carry any payload. It only notify you 
that something happen. Then this is up to the user to decide what to you with it.

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/22/19 3:33 PM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>> Discussing with my team, a solution that came up would be to 
>>> introduce one atomic field per event to record the number of event 
>>> received. I will explore that solution tomorrow.
>> How will this help if events have some payload?
>
> What payload? The event channel does not carry any payload. It only 
> notify you that something happen. Then this is up to the user to 
> decide what to you with it.
Sorry, I was probably not precise enough. I mean that an event might have
associated payload in the ring buffer, for example [1]. So, counting events
may help somehow, but the ring's data may still be lost
>
> Cheers,
>
[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/22/19 3:33 PM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>> Discussing with my team, a solution that came up would be to 
>>> introduce one atomic field per event to record the number of event 
>>> received. I will explore that solution tomorrow.
>> How will this help if events have some payload?
>
> What payload? The event channel does not carry any payload. It only 
> notify you that something happen. Then this is up to the user to 
> decide what to you with it.
Sorry, I was probably not precise enough. I mean that an event might have
associated payload in the ring buffer, for example [1]. So, counting events
may help somehow, but the ring's data may still be lost
>
> Cheers,
>
[1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Oleksandr,

On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> On 2/22/19 3:33 PM, Julien Grall wrote:
>> Hi,
>>
>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>> Discussing with my team, a solution that came up would be to introduce one 
>>>> atomic field per event to record the number of event received. I will 
>>>> explore that solution tomorrow.
>>> How will this help if events have some payload?
>>
>> What payload? The event channel does not carry any payload. It only notify you 
>> that something happen. Then this is up to the user to decide what to you with it.
> Sorry, I was probably not precise enough. I mean that an event might have
> associated payload in the ring buffer, for example [1]. So, counting events
> may help somehow, but the ring's data may still be lost

 From my understanding of event channels are edge interrupts. By definition, 
they can be merged so you can get a signal notification to the guest for 
multiple "events". So if you rely on the event to have an associated payload, 
then you probably have done something wrong in your driver.

I haven't implemented PV drivers myself, but I would expect either side to block 
if there were no space in the ring.

What do you do in the displif driver when the ring is full?

Cheers,

> [1] 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756 
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi Oleksandr,

On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> On 2/22/19 3:33 PM, Julien Grall wrote:
>> Hi,
>>
>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>> Discussing with my team, a solution that came up would be to introduce one 
>>>> atomic field per event to record the number of event received. I will 
>>>> explore that solution tomorrow.
>>> How will this help if events have some payload?
>>
>> What payload? The event channel does not carry any payload. It only notify you 
>> that something happen. Then this is up to the user to decide what to you with it.
> Sorry, I was probably not precise enough. I mean that an event might have
> associated payload in the ring buffer, for example [1]. So, counting events
> may help somehow, but the ring's data may still be lost

 From my understanding of event channels are edge interrupts. By definition, 
they can be merged so you can get a signal notification to the guest for 
multiple "events". So if you rely on the event to have an associated payload, 
then you probably have done something wrong in your driver.

I haven't implemented PV drivers myself, but I would expect either side to block 
if there were no space in the ring.

What do you do in the displif driver when the ring is full?

Cheers,

> [1] 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756 
> 

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/25/19 3:55 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>> Discussing with my team, a solution that came up would be to 
>>>>> introduce one atomic field per event to record the number of event 
>>>>> received. I will explore that solution tomorrow.
>>>> How will this help if events have some payload?
>>>
>>> What payload? The event channel does not carry any payload. It only 
>>> notify you that something happen. Then this is up to the user to 
>>> decide what to you with it.
>> Sorry, I was probably not precise enough. I mean that an event might 
>> have
>> associated payload in the ring buffer, for example [1]. So, counting 
>> events
>> may help somehow, but the ring's data may still be lost
>
> From my understanding of event channels are edge interrupts. By 
> definition, they can be merged so you can get a signal notification to 
> the guest for multiple "events". So if you rely on the event to have 
> an associated payload, then you probably have done something wrong in 
> your driver.
>
> I haven't implemented PV drivers myself, but I would expect either 
> side to block if there were no space in the ring.
>
> What do you do in the displif driver when the ring is full?
>
It is handled by the originator, the display backend in our case: it 
doesn't send
events if it sees that the ring will overflow. But I was worried about
such a generic change with counting number of events received and if this
really helps to recover in general case
> Cheers,
>
>> [1] 
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756 
>>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/25/19 3:55 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>> Discussing with my team, a solution that came up would be to 
>>>>> introduce one atomic field per event to record the number of event 
>>>>> received. I will explore that solution tomorrow.
>>>> How will this help if events have some payload?
>>>
>>> What payload? The event channel does not carry any payload. It only 
>>> notify you that something happen. Then this is up to the user to 
>>> decide what to you with it.
>> Sorry, I was probably not precise enough. I mean that an event might 
>> have
>> associated payload in the ring buffer, for example [1]. So, counting 
>> events
>> may help somehow, but the ring's data may still be lost
>
> From my understanding of event channels are edge interrupts. By 
> definition, they can be merged so you can get a signal notification to 
> the guest for multiple "events". So if you rely on the event to have 
> an associated payload, then you probably have done something wrong in 
> your driver.
>
> I haven't implemented PV drivers myself, but I would expect either 
> side to block if there were no space in the ring.
>
> What do you do in the displif driver when the ring is full?
>
It is handled by the originator, the display backend in our case: it 
doesn't send
events if it sees that the ring will overflow. But I was worried about
such a generic change with counting number of events received and if this
really helps to recover in general case
> Cheers,
>
>> [1] 
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/displif.h;h=cc5de9cb1f35dedc99c866d73d086b19e496852a;hb=HEAD#l756 
>>
>

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Oleksandr,

On 25/02/2019 14:08, Oleksandr Andrushchenko wrote:
> On 2/25/19 3:55 PM, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>> Discussing with my team, a solution that came up would be to introduce one 
>>>>>> atomic field per event to record the number of event received. I will 
>>>>>> explore that solution tomorrow.
>>>>> How will this help if events have some payload?
>>>>
>>>> What payload? The event channel does not carry any payload. It only notify 
>>>> you that something happen. Then this is up to the user to decide what to you 
>>>> with it.
>>> Sorry, I was probably not precise enough. I mean that an event might have
>>> associated payload in the ring buffer, for example [1]. So, counting events
>>> may help somehow, but the ring's data may still be lost
>>
>> From my understanding of event channels are edge interrupts. By definition, 
>> they can be merged so you can get a signal notification to the guest for 
>> multiple "events". So if you rely on the event to have an associated payload, 
>> then you probably have done something wrong in your driver.
>>
>> I haven't implemented PV drivers myself, but I would expect either side to 
>> block if there were no space in the ring.
>>
>> What do you do in the displif driver when the ring is full?
>>
> It is handled by the originator, the display backend in our case: it doesn't send
> events if it sees that the ring will overflow. But I was worried about
> such a generic change with counting number of events received and if this
> really helps to recover in general case

Well, I was originally looking at modifying only the /dev/evtchn driver but it 
turns out the event flow for Xen irqchip is not entirely correct.

A lot of Xen PV drivers will thread the handler and set IRQF_ONESHOT expecting 
the event to be masked until the event handler has ran. However, the flow we use 
does not deal with it and actually warn you may receive another event before 
executing the handler for the first event.

I have discussed with Marc Z. (one of the irqchip maintainers) about the issue. 
He suggested a different interrupt flow that I need to try out.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi Oleksandr,

On 25/02/2019 14:08, Oleksandr Andrushchenko wrote:
> On 2/25/19 3:55 PM, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>> Discussing with my team, a solution that came up would be to introduce one 
>>>>>> atomic field per event to record the number of event received. I will 
>>>>>> explore that solution tomorrow.
>>>>> How will this help if events have some payload?
>>>>
>>>> What payload? The event channel does not carry any payload. It only notify 
>>>> you that something happen. Then this is up to the user to decide what to you 
>>>> with it.
>>> Sorry, I was probably not precise enough. I mean that an event might have
>>> associated payload in the ring buffer, for example [1]. So, counting events
>>> may help somehow, but the ring's data may still be lost
>>
>> From my understanding of event channels are edge interrupts. By definition, 
>> they can be merged so you can get a signal notification to the guest for 
>> multiple "events". So if you rely on the event to have an associated payload, 
>> then you probably have done something wrong in your driver.
>>
>> I haven't implemented PV drivers myself, but I would expect either side to 
>> block if there were no space in the ring.
>>
>> What do you do in the displif driver when the ring is full?
>>
> It is handled by the originator, the display backend in our case: it doesn't send
> events if it sees that the ring will overflow. But I was worried about
> such a generic change with counting number of events received and if this
> really helps to recover in general case

Well, I was originally looking at modifying only the /dev/evtchn driver but it 
turns out the event flow for Xen irqchip is not entirely correct.

A lot of Xen PV drivers will thread the handler and set IRQF_ONESHOT expecting 
the event to be masked until the event handler has ran. However, the flow we use 
does not deal with it and actually warn you may receive another event before 
executing the handler for the first event.

I have discussed with Marc Z. (one of the irqchip maintainers) about the issue. 
He suggested a different interrupt flow that I need to try out.

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > Discussing with my team, a solution that came up would be to
> > > > > introduce one atomic field per event to record the number of
> > > > > event received. I will explore that solution tomorrow.
> > > > How will this help if events have some payload?
> > > 
> > > What payload? The event channel does not carry any payload. It only
> > > notify you that something happen. Then this is up to the user to
> > > decide what to you with it.
> > Sorry, I was probably not precise enough. I mean that an event might have
> > associated payload in the ring buffer, for example [1]. So, counting events
> > may help somehow, but the ring's data may still be lost
> 
> From my understanding of event channels are edge interrupts. By definition,

IMO event channels are active high level interrupts.

Let's take into account the following situation: you have an event
channel masked and the event channel pending bit (akin to the line on
bare metal) goes from low to high (0 -> 1), then you unmask the
interrupt and you get an event injected. If it was an edge interrupt
you wont get an event injected after unmasking, because you would
have lost the edge. I think the problem here is that Linux treats
event channels as edge interrupts, when they are actually level.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > Discussing with my team, a solution that came up would be to
> > > > > introduce one atomic field per event to record the number of
> > > > > event received. I will explore that solution tomorrow.
> > > > How will this help if events have some payload?
> > > 
> > > What payload? The event channel does not carry any payload. It only
> > > notify you that something happen. Then this is up to the user to
> > > decide what to you with it.
> > Sorry, I was probably not precise enough. I mean that an event might have
> > associated payload in the ring buffer, for example [1]. So, counting events
> > may help somehow, but the ring's data may still be lost
> 
> From my understanding of event channels are edge interrupts. By definition,

IMO event channels are active high level interrupts.

Let's take into account the following situation: you have an event
channel masked and the event channel pending bit (akin to the line on
bare metal) goes from low to high (0 -> 1), then you unmask the
interrupt and you get an event injected. If it was an edge interrupt
you wont get an event injected after unmasking, because you would
have lost the edge. I think the problem here is that Linux treats
event channels as edge interrupts, when they are actually level.

Roger.

Re: xen/evtchn and forced threaded irq

[Andrew Cooper]

On 26/02/2019 09:14, Roger Pau Monné wrote:
> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>> Discussing with my team, a solution that came up would be to
>>>>>> introduce one atomic field per event to record the number of
>>>>>> event received. I will explore that solution tomorrow.
>>>>> How will this help if events have some payload?
>>>> What payload? The event channel does not carry any payload. It only
>>>> notify you that something happen. Then this is up to the user to
>>>> decide what to you with it.
>>> Sorry, I was probably not precise enough. I mean that an event might have
>>> associated payload in the ring buffer, for example [1]. So, counting events
>>> may help somehow, but the ring's data may still be lost
>> From my understanding of event channels are edge interrupts. By definition,
> IMO event channels are active high level interrupts.
>
> Let's take into account the following situation: you have an event
> channel masked and the event channel pending bit (akin to the line on
> bare metal) goes from low to high (0 -> 1), then you unmask the
> interrupt and you get an event injected. If it was an edge interrupt
> you wont get an event injected after unmasking, because you would
> have lost the edge. I think the problem here is that Linux treats
> event channels as edge interrupts, when they are actually level.

Event channels are edge interrupts.  There are several very subtle bugs
to be had by software which treats them as line interrupts.

Most critically, if you fail to ack them, rebind them to a new vcpu, and
reenable interrupts, you don't get a new interrupt notification.  This
was the source of a 4 month bug when XenServer was moving from
classic-xen to PVOps where using irqbalance would cause dom0 to
occasionally lose interrupts.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Andrew Cooper]

On 26/02/2019 09:14, Roger Pau Monné wrote:
> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>> Discussing with my team, a solution that came up would be to
>>>>>> introduce one atomic field per event to record the number of
>>>>>> event received. I will explore that solution tomorrow.
>>>>> How will this help if events have some payload?
>>>> What payload? The event channel does not carry any payload. It only
>>>> notify you that something happen. Then this is up to the user to
>>>> decide what to you with it.
>>> Sorry, I was probably not precise enough. I mean that an event might have
>>> associated payload in the ring buffer, for example [1]. So, counting events
>>> may help somehow, but the ring's data may still be lost
>> From my understanding of event channels are edge interrupts. By definition,
> IMO event channels are active high level interrupts.
>
> Let's take into account the following situation: you have an event
> channel masked and the event channel pending bit (akin to the line on
> bare metal) goes from low to high (0 -> 1), then you unmask the
> interrupt and you get an event injected. If it was an edge interrupt
> you wont get an event injected after unmasking, because you would
> have lost the edge. I think the problem here is that Linux treats
> event channels as edge interrupts, when they are actually level.

Event channels are edge interrupts.  There are several very subtle bugs
to be had by software which treats them as line interrupts.

Most critically, if you fail to ack them, rebind them to a new vcpu, and
reenable interrupts, you don't get a new interrupt notification.  This
was the source of a 4 month bug when XenServer was moving from
classic-xen to PVOps where using irqbalance would cause dom0 to
occasionally lose interrupts.

~Andrew

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> On 26/02/2019 09:14, Roger Pau Monné wrote:
> > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> >> Hi Oleksandr,
> >>
> >> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> >>> On 2/22/19 3:33 PM, Julien Grall wrote:
> >>>> Hi,
> >>>>
> >>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> >>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
> >>>>>> Discussing with my team, a solution that came up would be to
> >>>>>> introduce one atomic field per event to record the number of
> >>>>>> event received. I will explore that solution tomorrow.
> >>>>> How will this help if events have some payload?
> >>>> What payload? The event channel does not carry any payload. It only
> >>>> notify you that something happen. Then this is up to the user to
> >>>> decide what to you with it.
> >>> Sorry, I was probably not precise enough. I mean that an event might have
> >>> associated payload in the ring buffer, for example [1]. So, counting events
> >>> may help somehow, but the ring's data may still be lost
> >> From my understanding of event channels are edge interrupts. By definition,
> > IMO event channels are active high level interrupts.
> >
> > Let's take into account the following situation: you have an event
> > channel masked and the event channel pending bit (akin to the line on
> > bare metal) goes from low to high (0 -> 1), then you unmask the
> > interrupt and you get an event injected. If it was an edge interrupt
> > you wont get an event injected after unmasking, because you would
> > have lost the edge. I think the problem here is that Linux treats
> > event channels as edge interrupts, when they are actually level.
> 
> Event channels are edge interrupts.  There are several very subtle bugs
> to be had by software which treats them as line interrupts.
> 
> Most critically, if you fail to ack them, rebind them to a new vcpu, and
> reenable interrupts, you don't get a new interrupt notification.  This
> was the source of a 4 month bug when XenServer was moving from
> classic-xen to PVOps where using irqbalance would cause dom0 to
> occasionally lose interrupts.

I would argue that you need to mask them first, rebind to a new vcpu
and unmask, and then you will get an interrupt notification, or this
should be fixed in Xen to work as you expect: trigger an interrupt
notification when moving an asserted event channel between CPUs.

Is there any document that describes how such non trivial things (like
moving between CPUs) work for event/level interrupts?

Maybe I'm being obtuse, but from the example I gave above it's quite
clear to me event channels don't get triggered based on edge changes,
but rather on the line level.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> On 26/02/2019 09:14, Roger Pau Monné wrote:
> > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> >> Hi Oleksandr,
> >>
> >> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> >>> On 2/22/19 3:33 PM, Julien Grall wrote:
> >>>> Hi,
> >>>>
> >>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> >>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
> >>>>>> Discussing with my team, a solution that came up would be to
> >>>>>> introduce one atomic field per event to record the number of
> >>>>>> event received. I will explore that solution tomorrow.
> >>>>> How will this help if events have some payload?
> >>>> What payload? The event channel does not carry any payload. It only
> >>>> notify you that something happen. Then this is up to the user to
> >>>> decide what to you with it.
> >>> Sorry, I was probably not precise enough. I mean that an event might have
> >>> associated payload in the ring buffer, for example [1]. So, counting events
> >>> may help somehow, but the ring's data may still be lost
> >> From my understanding of event channels are edge interrupts. By definition,
> > IMO event channels are active high level interrupts.
> >
> > Let's take into account the following situation: you have an event
> > channel masked and the event channel pending bit (akin to the line on
> > bare metal) goes from low to high (0 -> 1), then you unmask the
> > interrupt and you get an event injected. If it was an edge interrupt
> > you wont get an event injected after unmasking, because you would
> > have lost the edge. I think the problem here is that Linux treats
> > event channels as edge interrupts, when they are actually level.
> 
> Event channels are edge interrupts.  There are several very subtle bugs
> to be had by software which treats them as line interrupts.
> 
> Most critically, if you fail to ack them, rebind them to a new vcpu, and
> reenable interrupts, you don't get a new interrupt notification.  This
> was the source of a 4 month bug when XenServer was moving from
> classic-xen to PVOps where using irqbalance would cause dom0 to
> occasionally lose interrupts.

I would argue that you need to mask them first, rebind to a new vcpu
and unmask, and then you will get an interrupt notification, or this
should be fixed in Xen to work as you expect: trigger an interrupt
notification when moving an asserted event channel between CPUs.

Is there any document that describes how such non trivial things (like
moving between CPUs) work for event/level interrupts?

Maybe I'm being obtuse, but from the example I gave above it's quite
clear to me event channels don't get triggered based on edge changes,
but rather on the line level.

Thanks, Roger.

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi Roger,

On 26/02/2019 09:44, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>>
>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>> How will this help if events have some payload?
>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>> notify you that something happen. Then this is up to the user to
>>>>>> decide what to you with it.
>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>> may help somehow, but the ring's data may still be lost
>>>>  From my understanding of event channels are edge interrupts. By definition,
>>> IMO event channels are active high level interrupts.
>>>
>>> Let's take into account the following situation: you have an event
>>> channel masked and the event channel pending bit (akin to the line on
>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>> interrupt and you get an event injected. If it was an edge interrupt
>>> you wont get an event injected after unmasking, because you would
>>> have lost the edge. I think the problem here is that Linux treats
>>> event channels as edge interrupts, when they are actually level.
>>
>> Event channels are edge interrupts.  There are several very subtle bugs
>> to be had by software which treats them as line interrupts.
>>
>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>> reenable interrupts, you don't get a new interrupt notification.  This
>> was the source of a 4 month bug when XenServer was moving from
>> classic-xen to PVOps where using irqbalance would cause dom0 to
>> occasionally lose interrupts.
> 
> I would argue that you need to mask them first, rebind to a new vcpu
> and unmask, and then you will get an interrupt notification, or this
> should be fixed in Xen to work as you expect: trigger an interrupt
> notification when moving an asserted event channel between CPUs.
> 
> Is there any document that describes how such non trivial things (like
> moving between CPUs) work for event/level interrupts?
> 
> Maybe I'm being obtuse, but from the example I gave above it's quite
> clear to me event channels don't get triggered based on edge changes,
> but rather on the line level.

Your example above is not enough to give the semantics of level. You would only 
use the MASK bit if your interrupt handler is threaded to avoid the interrupt 
coming up again.

So if you remove the mask from the equation, then the interrupt flow should be:

1) handle interrupt
2) EOI

The EOI in our case would be clearing the PENDING state. In a proper level 
interrupt, the state would stay PENDING if there were more to come. This is not 
the case with the events and you will lose the interrupt.

So I don't think they are proper level interrupts. They have more a semantics of 
edge interrupts with some property of level (i.e for the mask/unmask).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi Roger,

On 26/02/2019 09:44, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>>
>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>> How will this help if events have some payload?
>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>> notify you that something happen. Then this is up to the user to
>>>>>> decide what to you with it.
>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>> may help somehow, but the ring's data may still be lost
>>>>  From my understanding of event channels are edge interrupts. By definition,
>>> IMO event channels are active high level interrupts.
>>>
>>> Let's take into account the following situation: you have an event
>>> channel masked and the event channel pending bit (akin to the line on
>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>> interrupt and you get an event injected. If it was an edge interrupt
>>> you wont get an event injected after unmasking, because you would
>>> have lost the edge. I think the problem here is that Linux treats
>>> event channels as edge interrupts, when they are actually level.
>>
>> Event channels are edge interrupts.  There are several very subtle bugs
>> to be had by software which treats them as line interrupts.
>>
>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>> reenable interrupts, you don't get a new interrupt notification.  This
>> was the source of a 4 month bug when XenServer was moving from
>> classic-xen to PVOps where using irqbalance would cause dom0 to
>> occasionally lose interrupts.
> 
> I would argue that you need to mask them first, rebind to a new vcpu
> and unmask, and then you will get an interrupt notification, or this
> should be fixed in Xen to work as you expect: trigger an interrupt
> notification when moving an asserted event channel between CPUs.
> 
> Is there any document that describes how such non trivial things (like
> moving between CPUs) work for event/level interrupts?
> 
> Maybe I'm being obtuse, but from the example I gave above it's quite
> clear to me event channels don't get triggered based on edge changes,
> but rather on the line level.

Your example above is not enough to give the semantics of level. You would only 
use the MASK bit if your interrupt handler is threaded to avoid the interrupt 
coming up again.

So if you remove the mask from the equation, then the interrupt flow should be:

1) handle interrupt
2) EOI

The EOI in our case would be clearing the PENDING state. In a proper level 
interrupt, the state would stay PENDING if there were more to come. This is not 
the case with the events and you will lose the interrupt.

So I don't think they are proper level interrupts. They have more a semantics of 
edge interrupts with some property of level (i.e for the mask/unmask).

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 26/02/2019 09:44, Roger Pau Monné wrote:
> > On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> > > On 26/02/2019 09:14, Roger Pau Monné wrote:
> > > > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> > > > > Hi Oleksandr,
> > > > > 
> > > > > On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > > > > > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > > > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > > > > > Discussing with my team, a solution that came up would be to
> > > > > > > > > introduce one atomic field per event to record the number of
> > > > > > > > > event received. I will explore that solution tomorrow.
> > > > > > > > How will this help if events have some payload?
> > > > > > > What payload? The event channel does not carry any payload. It only
> > > > > > > notify you that something happen. Then this is up to the user to
> > > > > > > decide what to you with it.
> > > > > > Sorry, I was probably not precise enough. I mean that an event might have
> > > > > > associated payload in the ring buffer, for example [1]. So, counting events
> > > > > > may help somehow, but the ring's data may still be lost
> > > > >  From my understanding of event channels are edge interrupts. By definition,
> > > > IMO event channels are active high level interrupts.
> > > > 
> > > > Let's take into account the following situation: you have an event
> > > > channel masked and the event channel pending bit (akin to the line on
> > > > bare metal) goes from low to high (0 -> 1), then you unmask the
> > > > interrupt and you get an event injected. If it was an edge interrupt
> > > > you wont get an event injected after unmasking, because you would
> > > > have lost the edge. I think the problem here is that Linux treats
> > > > event channels as edge interrupts, when they are actually level.
> > > 
> > > Event channels are edge interrupts.  There are several very subtle bugs
> > > to be had by software which treats them as line interrupts.
> > > 
> > > Most critically, if you fail to ack them, rebind them to a new vcpu, and
> > > reenable interrupts, you don't get a new interrupt notification.  This
> > > was the source of a 4 month bug when XenServer was moving from
> > > classic-xen to PVOps where using irqbalance would cause dom0 to
> > > occasionally lose interrupts.
> > 
> > I would argue that you need to mask them first, rebind to a new vcpu
> > and unmask, and then you will get an interrupt notification, or this
> > should be fixed in Xen to work as you expect: trigger an interrupt
> > notification when moving an asserted event channel between CPUs.
> > 
> > Is there any document that describes how such non trivial things (like
> > moving between CPUs) work for event/level interrupts?
> > 
> > Maybe I'm being obtuse, but from the example I gave above it's quite
> > clear to me event channels don't get triggered based on edge changes,
> > but rather on the line level.
> 
> Your example above is not enough to give the semantics of level. You would
> only use the MASK bit if your interrupt handler is threaded to avoid the
> interrupt coming up again.
> 
> So if you remove the mask from the equation, then the interrupt flow should be:
> 
> 1) handle interrupt
> 2) EOI

This is bogus if you don't mask the interrupt source. You should
instead do

1) EOI
2) Handle interrupt

And loop over this.

> The EOI in our case would be clearing the PENDING state. In a proper level
> interrupt, the state would stay PENDING if there were more to come. This is
> not the case with the events and you will lose the interrupt.
>
> So I don't think they are proper level interrupts. They have more a
> semantics of edge interrupts with some property of level (i.e for the
> mask/unmask).

OK, I guess it depends on how you look at it, to me event channels are
maybe quirky level interrupts, but are definitely closer to level than
edge interrupts, specially taking into account the interrupt injection
that happens on unmask of a pending line, there's no such thing at all
with edge interrupts.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 26/02/2019 09:44, Roger Pau Monné wrote:
> > On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> > > On 26/02/2019 09:14, Roger Pau Monné wrote:
> > > > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> > > > > Hi Oleksandr,
> > > > > 
> > > > > On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > > > > > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > > > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > > > > > Discussing with my team, a solution that came up would be to
> > > > > > > > > introduce one atomic field per event to record the number of
> > > > > > > > > event received. I will explore that solution tomorrow.
> > > > > > > > How will this help if events have some payload?
> > > > > > > What payload? The event channel does not carry any payload. It only
> > > > > > > notify you that something happen. Then this is up to the user to
> > > > > > > decide what to you with it.
> > > > > > Sorry, I was probably not precise enough. I mean that an event might have
> > > > > > associated payload in the ring buffer, for example [1]. So, counting events
> > > > > > may help somehow, but the ring's data may still be lost
> > > > >  From my understanding of event channels are edge interrupts. By definition,
> > > > IMO event channels are active high level interrupts.
> > > > 
> > > > Let's take into account the following situation: you have an event
> > > > channel masked and the event channel pending bit (akin to the line on
> > > > bare metal) goes from low to high (0 -> 1), then you unmask the
> > > > interrupt and you get an event injected. If it was an edge interrupt
> > > > you wont get an event injected after unmasking, because you would
> > > > have lost the edge. I think the problem here is that Linux treats
> > > > event channels as edge interrupts, when they are actually level.
> > > 
> > > Event channels are edge interrupts.  There are several very subtle bugs
> > > to be had by software which treats them as line interrupts.
> > > 
> > > Most critically, if you fail to ack them, rebind them to a new vcpu, and
> > > reenable interrupts, you don't get a new interrupt notification.  This
> > > was the source of a 4 month bug when XenServer was moving from
> > > classic-xen to PVOps where using irqbalance would cause dom0 to
> > > occasionally lose interrupts.
> > 
> > I would argue that you need to mask them first, rebind to a new vcpu
> > and unmask, and then you will get an interrupt notification, or this
> > should be fixed in Xen to work as you expect: trigger an interrupt
> > notification when moving an asserted event channel between CPUs.
> > 
> > Is there any document that describes how such non trivial things (like
> > moving between CPUs) work for event/level interrupts?
> > 
> > Maybe I'm being obtuse, but from the example I gave above it's quite
> > clear to me event channels don't get triggered based on edge changes,
> > but rather on the line level.
> 
> Your example above is not enough to give the semantics of level. You would
> only use the MASK bit if your interrupt handler is threaded to avoid the
> interrupt coming up again.
> 
> So if you remove the mask from the equation, then the interrupt flow should be:
> 
> 1) handle interrupt
> 2) EOI

This is bogus if you don't mask the interrupt source. You should
instead do

1) EOI
2) Handle interrupt

And loop over this.

> The EOI in our case would be clearing the PENDING state. In a proper level
> interrupt, the state would stay PENDING if there were more to come. This is
> not the case with the events and you will lose the interrupt.
>
> So I don't think they are proper level interrupts. They have more a
> semantics of edge interrupts with some property of level (i.e for the
> mask/unmask).

OK, I guess it depends on how you look at it, to me event channels are
maybe quirky level interrupts, but are definitely closer to level than
edge interrupts, specially taking into account the interrupt injection
that happens on unmask of a pending line, there's no such thing at all
with edge interrupts.

Thanks, Roger.

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 26/02/2019 10:17, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On 26/02/2019 09:44, Roger Pau Monné wrote:
>>> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>>>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>>>> Hi Oleksandr,
>>>>>>
>>>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>>>> How will this help if events have some payload?
>>>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>>>> notify you that something happen. Then this is up to the user to
>>>>>>>> decide what to you with it.
>>>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>>>> may help somehow, but the ring's data may still be lost
>>>>>>   From my understanding of event channels are edge interrupts. By definition,
>>>>> IMO event channels are active high level interrupts.
>>>>>
>>>>> Let's take into account the following situation: you have an event
>>>>> channel masked and the event channel pending bit (akin to the line on
>>>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>>>> interrupt and you get an event injected. If it was an edge interrupt
>>>>> you wont get an event injected after unmasking, because you would
>>>>> have lost the edge. I think the problem here is that Linux treats
>>>>> event channels as edge interrupts, when they are actually level.
>>>>
>>>> Event channels are edge interrupts.  There are several very subtle bugs
>>>> to be had by software which treats them as line interrupts.
>>>>
>>>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>>>> reenable interrupts, you don't get a new interrupt notification.  This
>>>> was the source of a 4 month bug when XenServer was moving from
>>>> classic-xen to PVOps where using irqbalance would cause dom0 to
>>>> occasionally lose interrupts.
>>>
>>> I would argue that you need to mask them first, rebind to a new vcpu
>>> and unmask, and then you will get an interrupt notification, or this
>>> should be fixed in Xen to work as you expect: trigger an interrupt
>>> notification when moving an asserted event channel between CPUs.
>>>
>>> Is there any document that describes how such non trivial things (like
>>> moving between CPUs) work for event/level interrupts?
>>>
>>> Maybe I'm being obtuse, but from the example I gave above it's quite
>>> clear to me event channels don't get triggered based on edge changes,
>>> but rather on the line level.
>>
>> Your example above is not enough to give the semantics of level. You would
>> only use the MASK bit if your interrupt handler is threaded to avoid the
>> interrupt coming up again.
>>
>> So if you remove the mask from the equation, then the interrupt flow should be:
>>
>> 1) handle interrupt
>> 2) EOI
> 
> This is bogus if you don't mask the interrupt source. You should
> instead do
> 
> 1) EOI
> 2) Handle interrupt
> 
> And loop over this.
So that's not a level semantics. It is a edge one :). In the level case, you 
would clear the state once you are done with the interrupt.

Also, it would be ACK and not EOI.

> 
>> The EOI in our case would be clearing the PENDING state. In a proper level
>> interrupt, the state would stay PENDING if there were more to come. This is
>> not the case with the events and you will lose the interrupt.
>>
>> So I don't think they are proper level interrupts. They have more a
>> semantics of edge interrupts with some property of level (i.e for the
>> mask/unmask).
> 
> OK, I guess it depends on how you look at it, to me event channels are
> maybe quirky level interrupts, but are definitely closer to level than
> edge interrupts, specially taking into account the interrupt injection
> that happens on unmask of a pending line, there's no such thing at all
> with edge interrupts.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 26/02/2019 10:17, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On 26/02/2019 09:44, Roger Pau Monné wrote:
>>> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>>>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>>>> Hi Oleksandr,
>>>>>>
>>>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>>>> How will this help if events have some payload?
>>>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>>>> notify you that something happen. Then this is up to the user to
>>>>>>>> decide what to you with it.
>>>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>>>> may help somehow, but the ring's data may still be lost
>>>>>>   From my understanding of event channels are edge interrupts. By definition,
>>>>> IMO event channels are active high level interrupts.
>>>>>
>>>>> Let's take into account the following situation: you have an event
>>>>> channel masked and the event channel pending bit (akin to the line on
>>>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>>>> interrupt and you get an event injected. If it was an edge interrupt
>>>>> you wont get an event injected after unmasking, because you would
>>>>> have lost the edge. I think the problem here is that Linux treats
>>>>> event channels as edge interrupts, when they are actually level.
>>>>
>>>> Event channels are edge interrupts.  There are several very subtle bugs
>>>> to be had by software which treats them as line interrupts.
>>>>
>>>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>>>> reenable interrupts, you don't get a new interrupt notification.  This
>>>> was the source of a 4 month bug when XenServer was moving from
>>>> classic-xen to PVOps where using irqbalance would cause dom0 to
>>>> occasionally lose interrupts.
>>>
>>> I would argue that you need to mask them first, rebind to a new vcpu
>>> and unmask, and then you will get an interrupt notification, or this
>>> should be fixed in Xen to work as you expect: trigger an interrupt
>>> notification when moving an asserted event channel between CPUs.
>>>
>>> Is there any document that describes how such non trivial things (like
>>> moving between CPUs) work for event/level interrupts?
>>>
>>> Maybe I'm being obtuse, but from the example I gave above it's quite
>>> clear to me event channels don't get triggered based on edge changes,
>>> but rather on the line level.
>>
>> Your example above is not enough to give the semantics of level. You would
>> only use the MASK bit if your interrupt handler is threaded to avoid the
>> interrupt coming up again.
>>
>> So if you remove the mask from the equation, then the interrupt flow should be:
>>
>> 1) handle interrupt
>> 2) EOI
> 
> This is bogus if you don't mask the interrupt source. You should
> instead do
> 
> 1) EOI
> 2) Handle interrupt
> 
> And loop over this.
So that's not a level semantics. It is a edge one :). In the level case, you 
would clear the state once you are done with the interrupt.

Also, it would be ACK and not EOI.

> 
>> The EOI in our case would be clearing the PENDING state. In a proper level
>> interrupt, the state would stay PENDING if there were more to come. This is
>> not the case with the events and you will lose the interrupt.
>>
>> So I don't think they are proper level interrupts. They have more a
>> semantics of edge interrupts with some property of level (i.e for the
>> mask/unmask).
> 
> OK, I guess it depends on how you look at it, to me event channels are
> maybe quirky level interrupts, but are definitely closer to level than
> edge interrupts, specially taking into account the interrupt injection
> that happens on unmask of a pending line, there's no such thing at all
> with edge interrupts.

Cheers,

-- 
Julien Grall

Re: xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 10:26:21AM +0000, Julien Grall wrote:
> Hi,
> 
> On 26/02/2019 10:17, Roger Pau Monné wrote:
> > On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 26/02/2019 09:44, Roger Pau Monné wrote:
> > > > On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> > > > > On 26/02/2019 09:14, Roger Pau Monné wrote:
> > > > > > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> > > > > > > Hi Oleksandr,
> > > > > > > 
> > > > > > > On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > > > > > > > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > > > > > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > > > > > > > Discussing with my team, a solution that came up would be to
> > > > > > > > > > > introduce one atomic field per event to record the number of
> > > > > > > > > > > event received. I will explore that solution tomorrow.
> > > > > > > > > > How will this help if events have some payload?
> > > > > > > > > What payload? The event channel does not carry any payload. It only
> > > > > > > > > notify you that something happen. Then this is up to the user to
> > > > > > > > > decide what to you with it.
> > > > > > > > Sorry, I was probably not precise enough. I mean that an event might have
> > > > > > > > associated payload in the ring buffer, for example [1]. So, counting events
> > > > > > > > may help somehow, but the ring's data may still be lost
> > > > > > >   From my understanding of event channels are edge interrupts. By definition,
> > > > > > IMO event channels are active high level interrupts.
> > > > > > 
> > > > > > Let's take into account the following situation: you have an event
> > > > > > channel masked and the event channel pending bit (akin to the line on
> > > > > > bare metal) goes from low to high (0 -> 1), then you unmask the
> > > > > > interrupt and you get an event injected. If it was an edge interrupt
> > > > > > you wont get an event injected after unmasking, because you would
> > > > > > have lost the edge. I think the problem here is that Linux treats
> > > > > > event channels as edge interrupts, when they are actually level.
> > > > > 
> > > > > Event channels are edge interrupts.  There are several very subtle bugs
> > > > > to be had by software which treats them as line interrupts.
> > > > > 
> > > > > Most critically, if you fail to ack them, rebind them to a new vcpu, and
> > > > > reenable interrupts, you don't get a new interrupt notification.  This
> > > > > was the source of a 4 month bug when XenServer was moving from
> > > > > classic-xen to PVOps where using irqbalance would cause dom0 to
> > > > > occasionally lose interrupts.
> > > > 
> > > > I would argue that you need to mask them first, rebind to a new vcpu
> > > > and unmask, and then you will get an interrupt notification, or this
> > > > should be fixed in Xen to work as you expect: trigger an interrupt
> > > > notification when moving an asserted event channel between CPUs.
> > > > 
> > > > Is there any document that describes how such non trivial things (like
> > > > moving between CPUs) work for event/level interrupts?
> > > > 
> > > > Maybe I'm being obtuse, but from the example I gave above it's quite
> > > > clear to me event channels don't get triggered based on edge changes,
> > > > but rather on the line level.
> > > 
> > > Your example above is not enough to give the semantics of level. You would
> > > only use the MASK bit if your interrupt handler is threaded to avoid the
> > > interrupt coming up again.
> > > 
> > > So if you remove the mask from the equation, then the interrupt flow should be:
> > > 
> > > 1) handle interrupt
> > > 2) EOI
> > 
> > This is bogus if you don't mask the interrupt source. You should
> > instead do
> > 
> > 1) EOI
> > 2) Handle interrupt
> > 
> > And loop over this.
> So that's not a level semantics. It is a edge one :). In the level case, you
> would clear the state once you are done with the interrupt.
> 
> Also, it would be ACK and not EOI.

For level triggered interrupts you have to somehow signal the device
to stop asserting the line, which doesn't happen for Xen devices
because they just signal interrupts to Xen, but don't have a way to
keep event channels asserted, so I agree that this is different from
traditional level interrupts because devices using event channels
don't have a way to keep lines asserted.

I guess the most similar native interrupt is MSI with masking
support?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Roger Pau Monné]

On Tue, Feb 26, 2019 at 10:26:21AM +0000, Julien Grall wrote:
> Hi,
> 
> On 26/02/2019 10:17, Roger Pau Monné wrote:
> > On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 26/02/2019 09:44, Roger Pau Monné wrote:
> > > > On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
> > > > > On 26/02/2019 09:14, Roger Pau Monné wrote:
> > > > > > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> > > > > > > Hi Oleksandr,
> > > > > > > 
> > > > > > > On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> > > > > > > > On 2/22/19 3:33 PM, Julien Grall wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> > > > > > > > > > On 2/20/19 10:46 PM, Julien Grall wrote:
> > > > > > > > > > > Discussing with my team, a solution that came up would be to
> > > > > > > > > > > introduce one atomic field per event to record the number of
> > > > > > > > > > > event received. I will explore that solution tomorrow.
> > > > > > > > > > How will this help if events have some payload?
> > > > > > > > > What payload? The event channel does not carry any payload. It only
> > > > > > > > > notify you that something happen. Then this is up to the user to
> > > > > > > > > decide what to you with it.
> > > > > > > > Sorry, I was probably not precise enough. I mean that an event might have
> > > > > > > > associated payload in the ring buffer, for example [1]. So, counting events
> > > > > > > > may help somehow, but the ring's data may still be lost
> > > > > > >   From my understanding of event channels are edge interrupts. By definition,
> > > > > > IMO event channels are active high level interrupts.
> > > > > > 
> > > > > > Let's take into account the following situation: you have an event
> > > > > > channel masked and the event channel pending bit (akin to the line on
> > > > > > bare metal) goes from low to high (0 -> 1), then you unmask the
> > > > > > interrupt and you get an event injected. If it was an edge interrupt
> > > > > > you wont get an event injected after unmasking, because you would
> > > > > > have lost the edge. I think the problem here is that Linux treats
> > > > > > event channels as edge interrupts, when they are actually level.
> > > > > 
> > > > > Event channels are edge interrupts.  There are several very subtle bugs
> > > > > to be had by software which treats them as line interrupts.
> > > > > 
> > > > > Most critically, if you fail to ack them, rebind them to a new vcpu, and
> > > > > reenable interrupts, you don't get a new interrupt notification.  This
> > > > > was the source of a 4 month bug when XenServer was moving from
> > > > > classic-xen to PVOps where using irqbalance would cause dom0 to
> > > > > occasionally lose interrupts.
> > > > 
> > > > I would argue that you need to mask them first, rebind to a new vcpu
> > > > and unmask, and then you will get an interrupt notification, or this
> > > > should be fixed in Xen to work as you expect: trigger an interrupt
> > > > notification when moving an asserted event channel between CPUs.
> > > > 
> > > > Is there any document that describes how such non trivial things (like
> > > > moving between CPUs) work for event/level interrupts?
> > > > 
> > > > Maybe I'm being obtuse, but from the example I gave above it's quite
> > > > clear to me event channels don't get triggered based on edge changes,
> > > > but rather on the line level.
> > > 
> > > Your example above is not enough to give the semantics of level. You would
> > > only use the MASK bit if your interrupt handler is threaded to avoid the
> > > interrupt coming up again.
> > > 
> > > So if you remove the mask from the equation, then the interrupt flow should be:
> > > 
> > > 1) handle interrupt
> > > 2) EOI
> > 
> > This is bogus if you don't mask the interrupt source. You should
> > instead do
> > 
> > 1) EOI
> > 2) Handle interrupt
> > 
> > And loop over this.
> So that's not a level semantics. It is a edge one :). In the level case, you
> would clear the state once you are done with the interrupt.
> 
> Also, it would be ACK and not EOI.

For level triggered interrupts you have to somehow signal the device
to stop asserting the line, which doesn't happen for Xen devices
because they just signal interrupts to Xen, but don't have a way to
keep event channels asserted, so I agree that this is different from
traditional level interrupts because devices using event channels
don't have a way to keep lines asserted.

I guess the most similar native interrupt is MSI with masking
support?

Roger.

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 2/26/19 11:02 AM, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 10:26:21AM +0000, Julien Grall wrote:
>> On 26/02/2019 10:17, Roger Pau Monné wrote:
>>> On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 26/02/2019 09:44, Roger Pau Monné wrote:
>>>>> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>>>>>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>>>>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>>>>>> Hi Oleksandr,
>>>>>>>>
>>>>>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>>>>>> How will this help if events have some payload?
>>>>>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>>>>>> notify you that something happen. Then this is up to the user to
>>>>>>>>>> decide what to you with it.
>>>>>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>>>>>> may help somehow, but the ring's data may still be lost
>>>>>>>>    From my understanding of event channels are edge interrupts. By definition,
>>>>>>> IMO event channels are active high level interrupts.
>>>>>>>
>>>>>>> Let's take into account the following situation: you have an event
>>>>>>> channel masked and the event channel pending bit (akin to the line on
>>>>>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>>>>>> interrupt and you get an event injected. If it was an edge interrupt
>>>>>>> you wont get an event injected after unmasking, because you would
>>>>>>> have lost the edge. I think the problem here is that Linux treats
>>>>>>> event channels as edge interrupts, when they are actually level.
>>>>>>
>>>>>> Event channels are edge interrupts.  There are several very subtle bugs
>>>>>> to be had by software which treats them as line interrupts.
>>>>>>
>>>>>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>>>>>> reenable interrupts, you don't get a new interrupt notification.  This
>>>>>> was the source of a 4 month bug when XenServer was moving from
>>>>>> classic-xen to PVOps where using irqbalance would cause dom0 to
>>>>>> occasionally lose interrupts.
>>>>>
>>>>> I would argue that you need to mask them first, rebind to a new vcpu
>>>>> and unmask, and then you will get an interrupt notification, or this
>>>>> should be fixed in Xen to work as you expect: trigger an interrupt
>>>>> notification when moving an asserted event channel between CPUs.
>>>>>
>>>>> Is there any document that describes how such non trivial things (like
>>>>> moving between CPUs) work for event/level interrupts?
>>>>>
>>>>> Maybe I'm being obtuse, but from the example I gave above it's quite
>>>>> clear to me event channels don't get triggered based on edge changes,
>>>>> but rather on the line level.
>>>>
>>>> Your example above is not enough to give the semantics of level. You would
>>>> only use the MASK bit if your interrupt handler is threaded to avoid the
>>>> interrupt coming up again.
>>>>
>>>> So if you remove the mask from the equation, then the interrupt flow should be:
>>>>
>>>> 1) handle interrupt
>>>> 2) EOI
>>>
>>> This is bogus if you don't mask the interrupt source. You should
>>> instead do
>>>
>>> 1) EOI
>>> 2) Handle interrupt
>>>
>>> And loop over this.
>> So that's not a level semantics. It is a edge one :). In the level case, you
>> would clear the state once you are done with the interrupt.
>>
>> Also, it would be ACK and not EOI.
> 
> For level triggered interrupts you have to somehow signal the device
> to stop asserting the line, which doesn't happen for Xen devices
> because they just signal interrupts to Xen, but don't have a way to
> keep event channels asserted, so I agree that this is different from
> traditional level interrupts because devices using event channels
> don't have a way to keep lines asserted.
> 
> I guess the most similar native interrupt is MSI with masking
> support?

I don't know enough about MSI with masking support to be able to draw a 
comparison :).

The flow I have been suggested to re-use in Linux is 
handle_fasteoi_ack_irq. I haven't yet had time to have a try at it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 2/26/19 11:02 AM, Roger Pau Monné wrote:
> On Tue, Feb 26, 2019 at 10:26:21AM +0000, Julien Grall wrote:
>> On 26/02/2019 10:17, Roger Pau Monné wrote:
>>> On Tue, Feb 26, 2019 at 10:03:38AM +0000, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 26/02/2019 09:44, Roger Pau Monné wrote:
>>>>> On Tue, Feb 26, 2019 at 09:30:07AM +0000, Andrew Cooper wrote:
>>>>>> On 26/02/2019 09:14, Roger Pau Monné wrote:
>>>>>>> On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
>>>>>>>> Hi Oleksandr,
>>>>>>>>
>>>>>>>> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 2/22/19 3:33 PM, Julien Grall wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
>>>>>>>>>>>> Discussing with my team, a solution that came up would be to
>>>>>>>>>>>> introduce one atomic field per event to record the number of
>>>>>>>>>>>> event received. I will explore that solution tomorrow.
>>>>>>>>>>> How will this help if events have some payload?
>>>>>>>>>> What payload? The event channel does not carry any payload. It only
>>>>>>>>>> notify you that something happen. Then this is up to the user to
>>>>>>>>>> decide what to you with it.
>>>>>>>>> Sorry, I was probably not precise enough. I mean that an event might have
>>>>>>>>> associated payload in the ring buffer, for example [1]. So, counting events
>>>>>>>>> may help somehow, but the ring's data may still be lost
>>>>>>>>    From my understanding of event channels are edge interrupts. By definition,
>>>>>>> IMO event channels are active high level interrupts.
>>>>>>>
>>>>>>> Let's take into account the following situation: you have an event
>>>>>>> channel masked and the event channel pending bit (akin to the line on
>>>>>>> bare metal) goes from low to high (0 -> 1), then you unmask the
>>>>>>> interrupt and you get an event injected. If it was an edge interrupt
>>>>>>> you wont get an event injected after unmasking, because you would
>>>>>>> have lost the edge. I think the problem here is that Linux treats
>>>>>>> event channels as edge interrupts, when they are actually level.
>>>>>>
>>>>>> Event channels are edge interrupts.  There are several very subtle bugs
>>>>>> to be had by software which treats them as line interrupts.
>>>>>>
>>>>>> Most critically, if you fail to ack them, rebind them to a new vcpu, and
>>>>>> reenable interrupts, you don't get a new interrupt notification.  This
>>>>>> was the source of a 4 month bug when XenServer was moving from
>>>>>> classic-xen to PVOps where using irqbalance would cause dom0 to
>>>>>> occasionally lose interrupts.
>>>>>
>>>>> I would argue that you need to mask them first, rebind to a new vcpu
>>>>> and unmask, and then you will get an interrupt notification, or this
>>>>> should be fixed in Xen to work as you expect: trigger an interrupt
>>>>> notification when moving an asserted event channel between CPUs.
>>>>>
>>>>> Is there any document that describes how such non trivial things (like
>>>>> moving between CPUs) work for event/level interrupts?
>>>>>
>>>>> Maybe I'm being obtuse, but from the example I gave above it's quite
>>>>> clear to me event channels don't get triggered based on edge changes,
>>>>> but rather on the line level.
>>>>
>>>> Your example above is not enough to give the semantics of level. You would
>>>> only use the MASK bit if your interrupt handler is threaded to avoid the
>>>> interrupt coming up again.
>>>>
>>>> So if you remove the mask from the equation, then the interrupt flow should be:
>>>>
>>>> 1) handle interrupt
>>>> 2) EOI
>>>
>>> This is bogus if you don't mask the interrupt source. You should
>>> instead do
>>>
>>> 1) EOI
>>> 2) Handle interrupt
>>>
>>> And loop over this.
>> So that's not a level semantics. It is a edge one :). In the level case, you
>> would clear the state once you are done with the interrupt.
>>
>> Also, it would be ACK and not EOI.
> 
> For level triggered interrupts you have to somehow signal the device
> to stop asserting the line, which doesn't happen for Xen devices
> because they just signal interrupts to Xen, but don't have a way to
> keep event channels asserted, so I agree that this is different from
> traditional level interrupts because devices using event channels
> don't have a way to keep lines asserted.
> 
> I guess the most similar native interrupt is MSI with masking
> support?

I don't know enough about MSI with masking support to be able to draw a 
comparison :).

The flow I have been suggested to re-use in Linux is 
handle_fasteoi_ack_irq. I haven't yet had time to have a try at it.

Cheers,

-- 
Julien Grall

RE: [Xen-devel] xen/evtchn and forced threaded irq

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Andrew Cooper
> Sent: 26 February 2019 09:30
> To: Roger Pau Monne <roger.pau@citrix.com>; Julien Grall <julien.grall@arm.com>
> Cc: Juergen Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; Oleksandr
> Andrushchenko <andr2000@gmail.com>; linux-kernel@vger.kernel.org; Jan Beulich <JBeulich@suse.com>;
> xen-devel <xen-devel@lists.xenproject.org>; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Dave P
> Martin <Dave.Martin@arm.com>
> Subject: Re: [Xen-devel] xen/evtchn and forced threaded irq
> 
> On 26/02/2019 09:14, Roger Pau Monné wrote:
> > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> >> Hi Oleksandr,
> >>
> >> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> >>> On 2/22/19 3:33 PM, Julien Grall wrote:
> >>>> Hi,
> >>>>
> >>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> >>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
> >>>>>> Discussing with my team, a solution that came up would be to
> >>>>>> introduce one atomic field per event to record the number of
> >>>>>> event received. I will explore that solution tomorrow.
> >>>>> How will this help if events have some payload?
> >>>> What payload? The event channel does not carry any payload. It only
> >>>> notify you that something happen. Then this is up to the user to
> >>>> decide what to you with it.
> >>> Sorry, I was probably not precise enough. I mean that an event might have
> >>> associated payload in the ring buffer, for example [1]. So, counting events
> >>> may help somehow, but the ring's data may still be lost
> >> From my understanding of event channels are edge interrupts. By definition,
> > IMO event channels are active high level interrupts.
> >
> > Let's take into account the following situation: you have an event
> > channel masked and the event channel pending bit (akin to the line on
> > bare metal) goes from low to high (0 -> 1), then you unmask the
> > interrupt and you get an event injected. If it was an edge interrupt
> > you wont get an event injected after unmasking, because you would
> > have lost the edge. I think the problem here is that Linux treats
> > event channels as edge interrupts, when they are actually level.
> 
> Event channels are edge interrupts.  There are several very subtle bugs
> to be had by software which treats them as line interrupts.

They are more subtle than that are they not? There is a single per-vcpu ACK which can cover multiple event channels.

  Paul

> 
> Most critically, if you fail to ack them, rebind them to a new vcpu, and
> reenable interrupts, you don't get a new interrupt notification.  This
> was the source of a 4 month bug when XenServer was moving from
> classic-xen to PVOps where using irqbalance would cause dom0 to
> occasionally lose interrupts.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Andrew Cooper
> Sent: 26 February 2019 09:30
> To: Roger Pau Monne <roger.pau@citrix.com>; Julien Grall <julien.grall@arm.com>
> Cc: Juergen Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; Oleksandr
> Andrushchenko <andr2000@gmail.com>; linux-kernel@vger.kernel.org; Jan Beulich <JBeulich@suse.com>;
> xen-devel <xen-devel@lists.xenproject.org>; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Dave P
> Martin <Dave.Martin@arm.com>
> Subject: Re: [Xen-devel] xen/evtchn and forced threaded irq
> 
> On 26/02/2019 09:14, Roger Pau Monné wrote:
> > On Mon, Feb 25, 2019 at 01:55:42PM +0000, Julien Grall wrote:
> >> Hi Oleksandr,
> >>
> >> On 25/02/2019 13:24, Oleksandr Andrushchenko wrote:
> >>> On 2/22/19 3:33 PM, Julien Grall wrote:
> >>>> Hi,
> >>>>
> >>>> On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> >>>>> On 2/20/19 10:46 PM, Julien Grall wrote:
> >>>>>> Discussing with my team, a solution that came up would be to
> >>>>>> introduce one atomic field per event to record the number of
> >>>>>> event received. I will explore that solution tomorrow.
> >>>>> How will this help if events have some payload?
> >>>> What payload? The event channel does not carry any payload. It only
> >>>> notify you that something happen. Then this is up to the user to
> >>>> decide what to you with it.
> >>> Sorry, I was probably not precise enough. I mean that an event might have
> >>> associated payload in the ring buffer, for example [1]. So, counting events
> >>> may help somehow, but the ring's data may still be lost
> >> From my understanding of event channels are edge interrupts. By definition,
> > IMO event channels are active high level interrupts.
> >
> > Let's take into account the following situation: you have an event
> > channel masked and the event channel pending bit (akin to the line on
> > bare metal) goes from low to high (0 -> 1), then you unmask the
> > interrupt and you get an event injected. If it was an edge interrupt
> > you wont get an event injected after unmasking, because you would
> > have lost the edge. I think the problem here is that Linux treats
> > event channels as edge interrupts, when they are actually level.
> 
> Event channels are edge interrupts.  There are several very subtle bugs
> to be had by software which treats them as line interrupts.

They are more subtle than that are they not? There is a single per-vcpu ACK which can cover multiple event channels.

  Paul

> 
> Most critically, if you fail to ack them, rebind them to a new vcpu, and
> reenable interrupts, you don't get a new interrupt notification.  This
> was the source of a 4 month bug when XenServer was moving from
> classic-xen to PVOps where using irqbalance would cause dom0 to
> occasionally lose interrupts.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:
> On 2/20/19 10:46 PM, Julien Grall wrote:
>> Discussing with my team, a solution that came up would be to introduce one 
>> atomic field per event to record the number of event received. I will explore 
>> that solution tomorrow.
> How will this help if events have some payload?

What payload? The event channel does not carry any payload. It only notify you 
that something happen. Then this is up to the user to decide what to you with it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Julien Grall]

(+ Andrew and Jan for feedback on the event channel interrupt)

Hi Boris,

Thank you for the your feedback.

On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
> On 2/20/19 1:05 PM, Julien Grall wrote:
>> Hi,
>>
>> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>>> Hi Boris,
>>>>
>>>> Thank you for your answer.
>>>>
>>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>>> started,
>>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>>
>>>>>> After some investigation, this is because the irq handler will now
>>>>>> be threaded.
>>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>>> the option
>>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>>> that has
>>>>>> not RT support).
>>>>>>
>>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>>> communicate with
>>>>>> xenstore.
>>>>>>
>>>>>>    From my understanding, this is happening because the interrupt
>>>>>> handler is now
>>>>>> run in a thread. So we can have the following happening.
>>>>>>
>>>>>>       Interrupt context            |     Interrupt thread
>>>>>>                                    |
>>>>>>       receive interrupt port 6     |
>>>>>>       clear the evtchn port        |
>>>>>>       set IRQF_RUNTHREAD            |
>>>>>>       kick interrupt thread        |
>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>                                    |    call evtchn_interrupt
>>>>>>       receive interrupt port 6     |
>>>>>>       clear the evtchn port        |
>>>>>>       set IRQF_RUNTHREAD           |
>>>>>>       kick interrupt thread        |
>>>>>>                                    |    disable interrupt port 6
>>>>>>                                    |    evtchn->enabled = false
>>>>>>                                    |    [....]
>>>>>>                                    |
>>>>>>                                    |    *** Handling the second
>>>>>> interrupt ***
>>>>>>                                    |    clear IRQF_RUNTHREAD
>>>>>>                                    |    call evtchn_interrupt
>>>>>>                                    |    WARN(...)
>>>>>>
>>>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>>>
>>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>>> need to
>>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>>> RT-Linux.
>>>>>>
>>>>>> 2) Remove the warning
>>>>>
>>>>> I think access to evtchn->enabled is racy so (with or without the
>>>>> warning) we can't use it reliably.
>>>>
>>>> Thinking about it, it would not be the only issue. The ring is sized
>>>> to contain only one instance of the same event. So if you receive
>>>> twice the event, you may overflow the ring.
>>>
>>> Hm... That's another argument in favor of "unthreading" the handler.
>>
>> I first thought it would be possible to unthread it. However,
>> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
>> so this cannot be used in an interrupt context.
>>
>> So I think "unthreading" the handler is not an option here.
> 
> That sounds like a different problem. I.e. there are two issues:
> * threaded interrupts don't work properly (races, ring overflow)
> * evtchn_interrupt() (threaded or not) has spin_lock(), which is not
> going to work for RT

I am afraid that's not correct, you can use spin_lock() in threaded 
interrupt handler.

> The first can be fixed by using non-threaded handlers.

The two are somewhat related, if you use a non-threaded handler then you 
are not going to help the RT case.

In general, the unthreaded solution should be used in the last resort.

>>
>>>
>>>>
>>>>>
>>>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>>> re-enabled).
>>>> What do you mean by queue? Is it queueing in the ring?
>>>
>>>
>>> No, I was thinking about having a new structure for deferred interrupts.
>>
>> Hmmm, I am not entirely sure what would be the structure here. Could
>> you expand your thinking?
> 
> Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
> implemented as a ring but not necessarily as Xen shared ring (if that's
> what you were referring to).

The underlying question is what happen if you miss an interrupt. Is it 
going to be ok? If no, then we have to record everything and can't 
kill/send an error to the user app because it is not its fault.

This means a FIFO would not be a viable. How do you size it? Static (i.e 
pre-defined) size is not going to be possible because you don't know how 
many interrupt you are going to receive before the thread handler runs. 
You can't make it grow dynamically as it make become quite big for the 
same reason.

Discussing with my team, a solution that came up would be to introduce 
one atomic field per event to record the number of event received. I 
will explore that solution tomorrow.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 1:05 PM, Julien Grall wrote:
> Hi,
>
> On 20/02/2019 17:07, Boris Ostrovsky wrote:
>> On 2/20/19 9:15 AM, Julien Grall wrote:
>>> Hi Boris,
>>>
>>> Thank you for your answer.
>>>
>>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>>> Hi all,
>>>>>
>>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>>> started,
>>>>> the console is ending to have a lot of warning (see trace below).
>>>>>
>>>>> After some investigation, this is because the irq handler will now
>>>>> be threaded.
>>>>> I can reproduce the same error with the vanilla Linux when passing
>>>>> the option
>>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>>> that has
>>>>> not RT support).
>>>>>
>>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>>> communicate with
>>>>> xenstore.
>>>>>
>>>>>   From my understanding, this is happening because the interrupt
>>>>> handler is now
>>>>> run in a thread. So we can have the following happening.
>>>>>
>>>>>      Interrupt context            |     Interrupt thread
>>>>>                                   |
>>>>>      receive interrupt port 6     |
>>>>>      clear the evtchn port        |
>>>>>      set IRQF_RUNTHREAD            |
>>>>>      kick interrupt thread        |
>>>>>                                   |    clear IRQF_RUNTHREAD
>>>>>                                   |    call evtchn_interrupt
>>>>>      receive interrupt port 6     |
>>>>>      clear the evtchn port        |
>>>>>      set IRQF_RUNTHREAD           |
>>>>>      kick interrupt thread        |
>>>>>                                   |    disable interrupt port 6
>>>>>                                   |    evtchn->enabled = false
>>>>>                                   |    [....]
>>>>>                                   |
>>>>>                                   |    *** Handling the second
>>>>> interrupt ***
>>>>>                                   |    clear IRQF_RUNTHREAD
>>>>>                                   |    call evtchn_interrupt
>>>>>                                   |    WARN(...)
>>>>>
>>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>>
>>>>> 1) Prevent the interrupt handler to be threaded. We would also
>>>>> need to
>>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>>> RT-Linux.
>>>>>
>>>>> 2) Remove the warning
>>>>
>>>> I think access to evtchn->enabled is racy so (with or without the
>>>> warning) we can't use it reliably.
>>>
>>> Thinking about it, it would not be the only issue. The ring is sized
>>> to contain only one instance of the same event. So if you receive
>>> twice the event, you may overflow the ring.
>>
>> Hm... That's another argument in favor of "unthreading" the handler.
>
> I first thought it would be possible to unthread it. However,
> wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
> so this cannot be used in an interrupt context.
>
> So I think "unthreading" the handler is not an option here.

That sounds like a different problem. I.e. there are two issues:
* threaded interrupts don't work properly (races, ring overflow)
* evtchn_interrupt() (threaded or not) has spin_lock(), which is not
going to work for RT

The first can be fixed by using non-threaded handlers.

>
>>
>>>
>>>>
>>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>>> handle it in evtchn_write() (which is where irq is supposed to be
>>>> re-enabled).
>>> What do you mean by queue? Is it queueing in the ring?
>>
>>
>> No, I was thinking about having a new structure for deferred interrupts.
>
> Hmmm, I am not entirely sure what would be the structure here. Could
> you expand your thinking?

Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
implemented as a ring but not necessarily as Xen shared ring (if that's
what you were referring to).

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 20/02/2019 17:07, Boris Ostrovsky wrote:
> On 2/20/19 9:15 AM, Julien Grall wrote:
>> Hi Boris,
>>
>> Thank you for your answer.
>>
>> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>>> Hi all,
>>>>
>>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>>> started,
>>>> the console is ending to have a lot of warning (see trace below).
>>>>
>>>> After some investigation, this is because the irq handler will now
>>>> be threaded.
>>>> I can reproduce the same error with the vanilla Linux when passing
>>>> the option
>>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>>> that has
>>>> not RT support).
>>>>
>>>> FWIW, the interrupt for port 6 is used to for the guest to
>>>> communicate with
>>>> xenstore.
>>>>
>>>>   From my understanding, this is happening because the interrupt
>>>> handler is now
>>>> run in a thread. So we can have the following happening.
>>>>
>>>>      Interrupt context            |     Interrupt thread
>>>>                                   |
>>>>      receive interrupt port 6     |
>>>>      clear the evtchn port        |
>>>>      set IRQF_RUNTHREAD            |
>>>>      kick interrupt thread        |
>>>>                                   |    clear IRQF_RUNTHREAD
>>>>                                   |    call evtchn_interrupt
>>>>      receive interrupt port 6     |
>>>>      clear the evtchn port        |
>>>>      set IRQF_RUNTHREAD           |
>>>>      kick interrupt thread        |
>>>>                                   |    disable interrupt port 6
>>>>                                   |    evtchn->enabled = false
>>>>                                   |    [....]
>>>>                                   |
>>>>                                   |    *** Handling the second
>>>> interrupt ***
>>>>                                   |    clear IRQF_RUNTHREAD
>>>>                                   |    call evtchn_interrupt
>>>>                                   |    WARN(...)
>>>>
>>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>>
>>>> 1) Prevent the interrupt handler to be threaded. We would also need to
>>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>>> RT-Linux.
>>>>
>>>> 2) Remove the warning
>>>
>>> I think access to evtchn->enabled is racy so (with or without the
>>> warning) we can't use it reliably.
>>
>> Thinking about it, it would not be the only issue. The ring is sized
>> to contain only one instance of the same event. So if you receive
>> twice the event, you may overflow the ring.
> 
> Hm... That's another argument in favor of "unthreading" the handler.

I first thought it would be possible to unthread it. However, 
wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep, so this 
cannot be used in an interrupt context.

So I think "unthreading" the handler is not an option here.

> 
>>
>>>
>>> Another alternative could be to queue the irq if !evtchn->enabled and
>>> handle it in evtchn_write() (which is where irq is supposed to be
>>> re-enabled).
>> What do you mean by queue? Is it queueing in the ring?
> 
> 
> No, I was thinking about having a new structure for deferred interrupts.

Hmmm, I am not entirely sure what would be the structure here. Could you expand 
your thinking?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On 2/20/19 9:15 AM, Julien Grall wrote:
> Hi Boris,
>
> Thank you for your answer.
>
> On 20/02/2019 00:02, Boris Ostrovsky wrote:
>> On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
>>> Hi all,
>>>
>>> I have been looking at using Linux RT in Dom0. Once the guest is
>>> started,
>>> the console is ending to have a lot of warning (see trace below).
>>>
>>> After some investigation, this is because the irq handler will now
>>> be threaded.
>>> I can reproduce the same error with the vanilla Linux when passing
>>> the option
>>> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7
>>> that has
>>> not RT support).
>>>
>>> FWIW, the interrupt for port 6 is used to for the guest to
>>> communicate with
>>> xenstore.
>>>
>>>  From my understanding, this is happening because the interrupt
>>> handler is now
>>> run in a thread. So we can have the following happening.
>>>
>>>     Interrupt context            |     Interrupt thread
>>>                                  |
>>>     receive interrupt port 6     |
>>>     clear the evtchn port        |
>>>     set IRQF_RUNTHREAD            |
>>>     kick interrupt thread        |
>>>                                  |    clear IRQF_RUNTHREAD
>>>                                  |    call evtchn_interrupt
>>>     receive interrupt port 6     |
>>>     clear the evtchn port        |
>>>     set IRQF_RUNTHREAD           |
>>>     kick interrupt thread        |
>>>                                  |    disable interrupt port 6
>>>                                  |    evtchn->enabled = false
>>>                                  |    [....]
>>>                                  |
>>>                                  |    *** Handling the second
>>> interrupt ***
>>>                                  |    clear IRQF_RUNTHREAD
>>>                                  |    call evtchn_interrupt
>>>                                  |    WARN(...)
>>>
>>> I am not entirely sure how to fix this. I have two solutions in mind:
>>>
>>> 1) Prevent the interrupt handler to be threaded. We would also need to
>>> switch from spin_lock to raw_spin_lock as the former may sleep on
>>> RT-Linux.
>>>
>>> 2) Remove the warning
>>
>> I think access to evtchn->enabled is racy so (with or without the
>> warning) we can't use it reliably.
>
> Thinking about it, it would not be the only issue. The ring is sized
> to contain only one instance of the same event. So if you receive
> twice the event, you may overflow the ring.

Hm... That's another argument in favor of "unthreading" the handler.

>
>>
>> Another alternative could be to queue the irq if !evtchn->enabled and
>> handle it in evtchn_write() (which is where irq is supposed to be
>> re-enabled).
> What do you mean by queue? Is it queueing in the ring? 


No, I was thinking about having a new structure for deferred interrupts.

-boris

> If so, wouldn't it be racy as described above?




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Boris Ostrovsky]

On Tue, Feb 19, 2019 at 05:31:10PM +0000, Julien Grall wrote:
> Hi all,
> 
> I have been looking at using Linux RT in Dom0. Once the guest is started,
> the console is ending to have a lot of warning (see trace below).
> 
> After some investigation, this is because the irq handler will now be threaded.
> I can reproduce the same error with the vanilla Linux when passing the option
> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
> not RT support).
> 
> FWIW, the interrupt for port 6 is used to for the guest to communicate with
> xenstore.
> 
> From my understanding, this is happening because the interrupt handler is now
> run in a thread. So we can have the following happening.
> 
>    Interrupt context            |     Interrupt thread
>                                 |  
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD	        |
>    kick interrupt thread        |
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD           |
>    kick interrupt thread        |
>                                 |    disable interrupt port 6
>                                 |    evtchn->enabled = false
>                                 |    [....]
>                                 |
>                                 |    *** Handling the second interrupt ***
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>                                 |    WARN(...)
> 
> I am not entirely sure how to fix this. I have two solutions in mind:
> 
> 1) Prevent the interrupt handler to be threaded. We would also need to
> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
> 
> 2) Remove the warning

I think access to evtchn->enabled is racy so (with or without the warning) we can't use it reliably.

Another alternative could be to queue the irq if !evtchn->enabled and handle it in evtchn_write() (which is where irq is supposed to be re-enabled).


-boris


> 
> None of them are ideals. Do you have an opionion/better suggestion?
> 
> [  127.192087] Interrupt for port 6, but apparently not enabled; per-user 0000000078d39c7f
> [  127.200333] WARNING: CPU: 0 PID: 2553 at drivers/xen/evtchn.c:167 evtchn_interrupt+0xfc/0x120
> [  127.208799] Modules linked in:
> [  127.211939] CPU: 0 PID: 2553 Comm: irq/52-evtchn:x Tainted: G        W
>   5.0.0-rc7-00023-g2a3d41623699 #1257
> [  127.222374] Hardware name: ARM Juno development board (r2) (DT)
> [  127.228381] pstate: 40000005 (nZcv daif -PAN -UAO)
> [  127.233256] pc : evtchn_interrupt+0xfc/0x120
> [  127.237607] lr : evtchn_interrupt+0xfc/0x120
> [  127.241952] sp : ffff000012d2bd60
> [  127.245347] x29: ffff000012d2bd60 x28: ffff00001015d608
> [  127.250741] x27: ffff8008b39de400 x26: ffff00001015d330
> [  127.256137] x25: ffff00001015d2d4 x24: 0000000000000001
> [  127.261532] x23: ffff00001015d570 x22: 0000000000000034
> [  127.266926] x21: 0000000000000000 x20: ffff8008b7f02400
> [  127.272322] x19: ffff8008b3aba000 x18: 0000000000000037
> [  127.277717] x17: 0000000000000000 x16: 0000000000000000
> [  127.283112] x15: 00000000fffffff0 x14: 0000000000000000
> [  127.288507] x13: 3030303030207265 x12: 000000000000000c
> [  127.293902] x11: ffff000010ea0a88 x10: 0000000000000000
> [  127.299297] x9 : 00000000fffb9fff x8 : 0000000000000000
> [  127.304701] x7 : 0000000000000001 x6 : ffff8008bac5c240
> [  127.310087] x5 : ffff8008bac5c240 x4 : 0000000000000000
> [  127.315483] x3 : ffff8008bac64708 x2 : ffff8008b39de400
> [  127.320877] x1 : 893ac9b38837b800 x0 : 0000000000000000
> [  127.326272] Call trace:
> [  127.328802]  evtchn_interrupt+0xfc/0x120
> [  127.332804]  irq_forced_thread_fn+0x38/0x98
> [  127.337066]  irq_thread+0x190/0x238
> [  127.340636]  kthread+0x134/0x138
> [  127.343942]  ret_from_fork+0x10/0x1c
> [  127.347593] ---[ end trace 1d3fa385877cc18b ]---
> 
> 
> Cheers,
> 
> -- 
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Juergen Gross]

On 19/02/2019 18:31, Julien Grall wrote:
> Hi all,
> 
> I have been looking at using Linux RT in Dom0. Once the guest is started,
> the console is ending to have a lot of warning (see trace below).
> 
> After some investigation, this is because the irq handler will now be threaded.
> I can reproduce the same error with the vanilla Linux when passing the option
> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
> not RT support).
> 
> FWIW, the interrupt for port 6 is used to for the guest to communicate with
> xenstore.
> 
> From my understanding, this is happening because the interrupt handler is now
> run in a thread. So we can have the following happening.
> 
>    Interrupt context            |     Interrupt thread
>                                 |  
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD	        |
>    kick interrupt thread        |
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD           |
>    kick interrupt thread        |
>                                 |    disable interrupt port 6
>                                 |    evtchn->enabled = false
>                                 |    [....]
>                                 |
>                                 |    *** Handling the second interrupt ***
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>                                 |    WARN(...)
> 
> I am not entirely sure how to fix this. I have two solutions in mind:
> 
> 1) Prevent the interrupt handler to be threaded. We would also need to
> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
> 
> 2) Remove the warning

3) Split the handler (RT-only?) to a non-threaded part containing
   everything until the "evtchn->enabled = false" and only then
   kick the thread doing the rest, including the spin_lock().

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: xen/evtchn and forced threaded irq

[Juergen Gross]

On 19/02/2019 18:31, Julien Grall wrote:
> Hi all,
> 
> I have been looking at using Linux RT in Dom0. Once the guest is started,
> the console is ending to have a lot of warning (see trace below).
> 
> After some investigation, this is because the irq handler will now be threaded.
> I can reproduce the same error with the vanilla Linux when passing the option
> 'threadirqs' on the command line (the trace below is from 5.0.0-rc7 that has
> not RT support).
> 
> FWIW, the interrupt for port 6 is used to for the guest to communicate with
> xenstore.
> 
> From my understanding, this is happening because the interrupt handler is now
> run in a thread. So we can have the following happening.
> 
>    Interrupt context            |     Interrupt thread
>                                 |  
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD	        |
>    kick interrupt thread        |
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>    receive interrupt port 6     |
>    clear the evtchn port        |
>    set IRQF_RUNTHREAD           |
>    kick interrupt thread        |
>                                 |    disable interrupt port 6
>                                 |    evtchn->enabled = false
>                                 |    [....]
>                                 |
>                                 |    *** Handling the second interrupt ***
>                                 |    clear IRQF_RUNTHREAD
>                                 |    call evtchn_interrupt
>                                 |    WARN(...)
> 
> I am not entirely sure how to fix this. I have two solutions in mind:
> 
> 1) Prevent the interrupt handler to be threaded. We would also need to
> switch from spin_lock to raw_spin_lock as the former may sleep on RT-Linux.
> 
> 2) Remove the warning

3) Split the handler (RT-only?) to a non-threaded part containing
   everything until the "evtchn->enabled = false" and only then
   kick the thread doing the rest, including the spin_lock().

Juergen