Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing

[Andrii Anisov <andrii.anisov@gmail.com>]

Hello Julien,

On 28.05.19 20:07, Julien Grall wrote:
> Title: Interrupts are still unmasked when executing action for interrupt routed to Xen. So you need to be more specific. How about
> "xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()"?

Looks good.

> 
> On 5/27/19 10:29 AM, Andrii Anisov wrote:
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> This reduces the number of context switches in case we have coming guest
>> interrupts from different sources at a high rate. What is likely for
> 
> s/What/This/
> 
>> multimedia use-cases.
>> Having irqs unlocked here makes us go through trap path again in case we
> 
> what do you mean by "unlocked"?

It must be "enabled".

>> have a new guest interrupt arrived (even with the same priority, after
>> `desc->handler->end(desc)` in `do_IRQ()`), what is just a processor
>> cycles wasting.
> after `desc->....`. This is just a waste a processor cycle as we will catch them all in the function gic_interrupt() loop.
> 
>   We will catch them all in the `gic_interrupt() function
>> loop anyway. And the guest irqs arrival prioritization is meaningless
>> here, it is only effective at guest's level.
> 
> I am not sure why you speak about guest prioritization here.

I'm trying to say about guest interrupts prioritization in HW. But I can drop it from the commit message.

> The main issue would be an interrupt to Xen (i.e timer) that would get delayed because of longer period without interrupt enabled.

Here we will process it on the next loop. This should not be much longer than existing vgic_inject_irq() interrupts disabled period.

> I would also not rule out the possibility to prioritize guest interrupt at hardware level.> 
> I know we have been discussing on the problem in the past,

Now I'm trying to pick the worthy bits from [1].
BTW, do you hear about plans for the new vgic? Some time ago it was said that new vgic implementation going to replace the old one, and optimizing the old is worthless. But as I see, there are no updates into that area yet.

> but a summary in the commit message is quite important to not miss out all the problems.

> The real problem here is for interrupt routed to guest the interrupt will be kept unmasked when calling desc->handler->end(desc). This will result to receive the next interrupt as soon as desc->handler->end(desc) is called.
> 
> In the case of interrupt routed to Xen, interrupts will be kept enabled while executing the action but then disabled before calling desc->handler->end(desc).
> 
> It would be fine to keep the interrupts masked for interrupts routed to the guest because vgic_inject_irq(...) will be masking the interrupt in most of the cases.
> 
> The code below looks good to me. I am happy to help rewording the commit message if necessary.

It's good to hear. I'm ready to reword the commit message as required to get the stuff upstreamed.
I'd discuss the wordings here. With changes suggested by you, the commit title and message would be following:

     xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}()

     This reduces the number of context switches in case we have coming guest
     interrupts from different sources at a high rate. That is likely for
     multimedia use-cases.
     Having irqs enabled here makes us go through trap path again in case we
     have a new guest interrupt arrived (even with the same or lower priority,
     after `desc->handler->end(desc)` in `do_IRQ()`), that is just a processor
     cycles wasting as we will catch them all in the `gic_interrupt() function
     loop.

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg02297.html

-- 
Sincerely,
Andrii Anisov.

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