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

[Julien Grall <julien.grall@xxxxxxx>]

On 29/05/2019 11:31, Andrii Anisov wrote:
> Hello Julien,
Hi,

> 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@xxxxxxxx>
> > >
> > > 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.
This should be explained in the commit message.

> > 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.
We need help to make it happen.

> > 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:
It would have been nice to at least fix up the commit message with the typoes 
(and rewording) I mentioned in my previous e-mail.
>      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
context switches is a bit confusing here. Do you mean trap?

Also s/coming/incoming/ or better "in case guest interrupts are received at high 
rate".
>      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.
Your commit message needs to explained why this is fine to keep the interrupt 
masked a bit longer. I wrote the explanation in my previous e-mail so you can 
borrow the rationale from there.
Cheers,

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

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