| Message ID | 1558949370-14331-1-git-send-email-andrii.anisov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] gic: drop interrupts enabling on interrupts processing | expand |
(+ Andre)
Hi,
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}()"?
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"?
> 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. The main
issue would be an interrupt to Xen (i.e timer) that would get delayed
because of longer period without interrupt enabled. 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, 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.
While looking at the code, I noticed that in the new vgic vgic_get_irq()
looks unsafe to be called with interrupt unmasked. This is because one
of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq.
Andre, what do you think?
>
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>
> Changes:
>
> in v2: Drop irq enabling for lpi processing as well.
>
>
> ---
> xen/arch/arm/gic.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6cc7dec..113655a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>
> if ( likely(irq >= 16 && irq < 1020) )
> {
> - local_irq_enable();
> isb();
> do_IRQ(regs, irq, is_fiq);
> - local_irq_disable();
> }
> else if ( is_lpi(irq) )
> {
> - local_irq_enable();
> isb();
> gic_hw_ops->do_LPI(irq);
> - local_irq_disable();
> }
> else if ( unlikely(irq < 16) )
> {
>
Cheers,
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
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@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. 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 >
On 29.05.19 18:32, Julien Grall wrote: > 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. > 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. xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}() Having irqs enabled here leaves a room for trapping and going through the trap path again if we have a new guest interrupt arrived (even with the same or lower priority, after `desc->handler->end(desc)` in `do_IRQ()`). Keeping interrupts disabled during guest interrupts processing allows as avoiding excessive traps (and wasting cpu cycles for trap path) while the new interrupts would be processed in the loop anyway. Processing guest interrupts by the loop should not introduce significant additional latency because vgic_inject_irq(...) already masking the interrupts in most of the cases.
On 29.05.19 18:32, Julien Grall wrote: >> 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. I'm not sure I'll have spare time soon, but what kind of help you need? Do you have a TODO list?
Hi Andrii, On 30/05/2019 17:12, Andrii Anisov wrote: > On 29.05.19 18:32, Julien Grall wrote: >> 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. >> 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. > xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}() > > Having irqs enabled here leaves a room for trapping and going through the trap Please avoid "here" in commit message if you haven't defined where is the issue. > path again if we have a new guest interrupt arrived (even with the same or I don't understand the "new guest interrupt arrived". > lower priority, after `desc->handler->end(desc)` in `do_IRQ()`). > Keeping interrupts disabled during guest interrupts processing allows as Missing word because "allows" and "as"? > avoiding excessive traps (and wasting cpu cycles for trap path) while the new > interrupts would be processed in the loop anyway. Processing guest interrupts by > the loop should not introduce significant additional latency because I am always worry when I see the word "should not" associated with "latency" because often it is actually the contrary (see the recent attempt to optimize the old vGIC). If you don't have number, then you should detail the rationale here. The more I think about it, the more I feel it would just be best to mask the interrupt just before dropping the priority. But I am happy to consider this if you have some ground to back the approach (they should be part of the commit message). > vgic_inject_irq(...) already masking the interrupts in most of the cases. Here my take on the commit message: gic_interrupt() was implemented using a loop to limit the cost of the trap if there are multiple interrupts pending. At the moment, interrupts are unmasked by gic_interrupt() before calling do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its priority will be dropped, via desc->handler->end() called from do_irq(), with interrupt unmasked. In other words: - Until the priority is dropped, only higher priority interrupt can be received. Today, only Xen interrupts have higher priority. - As soon as priority is dropped, any interrupt can be received. This means the purpose of the loop in gic_interrupt() is defeated as all interrupts may get trapped earlier. To reinstate the purpose of the loop (and prevent the trap), interrupts should be masked when dropping the priority. For interrupts routed to Xen, priority will always be dropped with interrupts masked. So the issue is not present. However, it means that we are pointless try to mask the interrupts. To avoid conflicting behavior between interrupt handling, gic_interrupt() is now keeping interrupts masked and defer the decision to do_{LPI, IRQ}. [ Details to be added once you give more ground ] Cheers,
Hi, On 30/05/2019 17:14, Andrii Anisov wrote: > > > On 29.05.19 18:32, Julien Grall wrote: >>> 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. > I'm not sure I'll have spare time soon, but what kind of help you need? Do you > have a TODO list? From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly need to be ported from Linux. There were also a couple of concern regarding using ordering the list while folding (can't remember if this was addressed in Linux). @Andre, I thought we capture that in Xen Project jira but I can't find it. Did you keep a list? Cheers,
On Tue, 28 May 2019 18:07:19 +0100 Julien Grall <julien.grall@arm.com> wrote: [ ... ] > While looking at the code, I noticed that in the new vgic vgic_get_irq() > looks unsafe to be called with interrupt unmasked. This is because one > of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. > Andre, what do you think? I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment. Cheers, Andre. > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > --- > > > > Changes: > > > > in v2: Drop irq enabling for lpi processing as well. > > > > > > --- > > xen/arch/arm/gic.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index 6cc7dec..113655a 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > > > > if ( likely(irq >= 16 && irq < 1020) ) > > { > > - local_irq_enable(); > > isb(); > > do_IRQ(regs, irq, is_fiq); > > - local_irq_disable(); > > } > > else if ( is_lpi(irq) ) > > { > > - local_irq_enable(); > > isb(); > > gic_hw_ops->do_LPI(irq); > > - local_irq_disable(); > > } > > else if ( unlikely(irq < 16) ) > > { > > > > Cheers, >
On Fri, 31 May 2019 18:16:52 +0100 Julien Grall <julien.grall@arm.com> wrote: > Hi, > > On 30/05/2019 17:14, Andrii Anisov wrote: > > > > > > On 29.05.19 18:32, Julien Grall wrote: > >>> 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. > > I'm not sure I'll have spare time soon, but what kind of help you need? Do you > > have a TODO list? > > From the top of my head the major blocker is GICv3 (+ ITS) support. It mostly > need to be ported from Linux. > > There were also a couple of concern regarding using ordering the list while > folding (can't remember if this was addressed in Linux). > > @Andre, I thought we capture that in Xen Project jira but I can't find it. Did > you keep a list? I thought Jira as well, but apparently we never actually did this :-( So yes, GICv3 support is the showstopper here, this would allow us to make the new VGIC the default, since we would have feature parity. Also we would need to check the git log in Linux for the individual VGIC files to port over fixes, if applicable. Due to the different coding style and renamed identifiers, comparing the files does not really help. Cheers, Andre.
On 31/05/2019 18:25, Andre Przywara wrote: > On Tue, 28 May 2019 18:07:19 +0100 > Julien Grall <julien.grall@arm.com> wrote: > > [ ... ] > >> While looking at the code, I noticed that in the new vgic vgic_get_irq() >> looks unsafe to be called with interrupt unmasked. This is because one >> of the callee (vgic_get_lpi()) takes a spinlock and not a spinlock_irq. >> Andre, what do you think? > > I think you are right. In vgic_inject_irq(), right after the call to vgic_get_irq(), we use spin_lock_irqsave() on the irq_lock, so using the same irqsave version on the lpi_list_lock seems needed. But this is somewhat theoretical at the moment, as I think we will never LPIs through the new VGIC at the moment. That's correct, we probably want to add that in the list of TODOs for the new vGIC :). Cheers,
On Fri, 31 May 2019, Julien Grall wrote: > Hi Andrii, > > On 30/05/2019 17:12, Andrii Anisov wrote: > > On 29.05.19 18:32, Julien Grall wrote: > > > 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. > > > 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. > > xen/arm: gic: Defer the decision to unmask interrupts to do_{LPI, IRQ}() > > > > Having irqs enabled here leaves a room for trapping and going through the > > trap > > Please avoid "here" in commit message if you haven't defined where is the > issue. > > > path again if we have a new guest interrupt arrived (even with the same or > > I don't understand the "new guest interrupt arrived". > > > lower priority, after `desc->handler->end(desc)` in `do_IRQ()`). > > Keeping interrupts disabled during guest interrupts processing allows as > > Missing word because "allows" and "as"? > > > avoiding excessive traps (and wasting cpu cycles for trap path) while the > > new > > interrupts would be processed in the loop anyway. Processing guest > > interrupts by > > the loop should not introduce significant additional latency because > > I am always worry when I see the word "should not" associated with "latency" > because often it is actually the contrary (see the recent attempt to optimize > the old vGIC). If you don't have number, then you should detail the rationale > here. > > The more I think about it, the more I feel it would just be best to mask the > interrupt just before dropping the priority. But I am happy to consider this > if you have some ground to back the approach (they should be part of the > commit message). > > > vgic_inject_irq(...) already masking the interrupts in most of the cases. > > Here my take on the commit message: > > gic_interrupt() was implemented using a loop to limit the cost of the trap if > there are multiple interrupts pending. > > At the moment, interrupts are unmasked by gic_interrupt() before calling > do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its > priority will be dropped, via desc->handler->end() called from do_irq(), with > interrupt unmasked. > > In other words: > - Until the priority is dropped, only higher priority interrupt can be > received. Today, only Xen interrupts have higher priority. > - As soon as priority is dropped, any interrupt can be received. > > This means the purpose of the loop in gic_interrupt() is defeated as all > interrupts may get trapped earlier. To reinstate the purpose of the loop (and > prevent the trap), interrupts should be masked when dropping the priority. > > For interrupts routed to Xen, priority will always be dropped with interrupts > masked. So the issue is not present. However, it means that we are pointless > try to mask the interrupts. > > To avoid conflicting behavior between interrupt handling, gic_interrupt() is > now keeping interrupts masked and defer the decision to do_{LPI, IRQ}. This is a really well written commit message, and together with the patch it looks fine to me.
Hello Julien, On 31.05.19 20:11, Julien Grall wrote: > Here my take on the commit message: > > gic_interrupt() was implemented using a loop to limit the cost of the trap if there are multiple interrupts pending. > > At the moment, interrupts are unmasked by gic_interrupt() before calling do_{IRQ, LPI}(). In the case of handling an interrupt routed to guests, its priority will be dropped, via desc->handler->end() called from do_irq(), with interrupt unmasked. > > In other words: > - Until the priority is dropped, only higher priority interrupt can be received. Today, only Xen interrupts have higher priority. > - As soon as priority is dropped, any interrupt can be received. > > This means the purpose of the loop in gic_interrupt() is defeated as all interrupts may get trapped earlier. To reinstate the purpose of the loop (and prevent the trap), interrupts should be masked when dropping the priority. > > For interrupts routed to Xen, priority will always be dropped with interrupts masked. So the issue is not present. However, it means that we are pointless try to mask the interrupts. > > To avoid conflicting behavior between interrupt handling, gic_interrupt() is now keeping interrupts masked and defer the decision to do_{LPI, IRQ}. It is OK with me. Are you waiting from me more of > [ Details to be added once you give more ground ] ?
On 6/10/19 4:49 PM, Andrii Anisov wrote: > Hello Julien, Hi Andrii, > > On 31.05.19 20:11, Julien Grall wrote: > >> Here my take on the commit message: >> >> gic_interrupt() was implemented using a loop to limit the cost of the >> trap if there are multiple interrupts pending. >> >> At the moment, interrupts are unmasked by gic_interrupt() before >> calling do_{IRQ, LPI}(). In the case of handling an interrupt routed >> to guests, its priority will be dropped, via desc->handler->end() >> called from do_irq(), with interrupt unmasked. >> >> In other words: >> - Until the priority is dropped, only higher priority interrupt >> can be received. Today, only Xen interrupts have higher priority. >> - As soon as priority is dropped, any interrupt can be received. >> >> This means the purpose of the loop in gic_interrupt() is defeated as >> all interrupts may get trapped earlier. To reinstate the purpose of >> the loop (and prevent the trap), interrupts should be masked when >> dropping the priority. >> >> For interrupts routed to Xen, priority will always be dropped with >> interrupts masked. So the issue is not present. However, it means that >> we are pointless try to mask the interrupts. >> >> To avoid conflicting behavior between interrupt handling, >> gic_interrupt() is now keeping interrupts masked and defer the >> decision to do_{LPI, IRQ}. > > It is OK with me. > > Are you waiting from me more of I was. But I also had time to think about the commit message and I would be happy to commit with just what it is currently written. I have now applied to my staging branch with my acked-by. I will commit it tonight. Thank you for the patch,
On 10.06.19 22:51, Julien Grall wrote: > I was. But I also had time to think about the commit message and I would be happy to commit with just what it is currently written. > > I have now applied to my staging branch with my acked-by. I will commit it tonight. Great. Thank you. > > Thank you for the patch,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 6cc7dec..113655a 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -386,17 +386,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) if ( likely(irq >= 16 && irq < 1020) ) { - local_irq_enable(); isb(); do_IRQ(regs, irq, is_fiq); - local_irq_disable(); } else if ( is_lpi(irq) ) { - local_irq_enable(); isb(); gic_hw_ops->do_LPI(irq); - local_irq_disable(); } else if ( unlikely(irq < 16) ) {