* [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-27 9:29 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-27 9:29 UTC (permalink / raw) To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Andrii Anisov 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 multimedia use-cases. Having irqs unlocked here makes us go through trap path again in case we 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. 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. 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) ) { -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-27 9:29 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-27 9:29 UTC (permalink / raw) To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Andrii Anisov 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 multimedia use-cases. Having irqs unlocked here makes us go through trap path again in case we 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. 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. 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) ) { -- 2.7.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-28 17:07 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-28 17:07 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov (+ 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-28 17:07 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-28 17:07 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov (+ 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-29 10:31 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-29 10:31 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-29 10:31 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-29 10:31 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-29 15:32 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-29 15:32 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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 > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-29 15:32 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-29 15:32 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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 > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-30 16:12 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-30 16:12 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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. -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-30 16:12 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-30 16:12 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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. -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:11 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:11 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:11 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:11 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 20:08 ` Stefano Stabellini 0 siblings, 0 replies; 27+ messages in thread From: Stefano Stabellini @ 2019-05-31 20:08 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Andre Przywara, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 20:08 ` Stefano Stabellini 0 siblings, 0 replies; 27+ messages in thread From: Stefano Stabellini @ 2019-05-31 20:08 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Andre Przywara, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing 2019-05-31 17:11 ` [Xen-devel] " Julien Grall (?) (?) @ 2019-06-10 15:49 ` Andrii Anisov 2019-06-10 19:51 ` Julien Grall -1 siblings, 1 reply; 27+ messages in thread From: Andrii Anisov @ 2019-06-10 15:49 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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 ] ? -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing 2019-06-10 15:49 ` Andrii Anisov @ 2019-06-10 19:51 ` Julien Grall 2019-06-11 7:38 ` Andrii Anisov 0 siblings, 1 reply; 27+ messages in thread From: Julien Grall @ 2019-06-10 19:51 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing 2019-06-10 19:51 ` Julien Grall @ 2019-06-11 7:38 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-06-11 7:38 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-30 16:14 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-30 16:14 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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? -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-30 16:14 ` Andrii Anisov 0 siblings, 0 replies; 27+ messages in thread From: Andrii Anisov @ 2019-05-30 16:14 UTC (permalink / raw) To: Julien Grall, xen-devel; +Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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? -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:16 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:16 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:16 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:16 UTC (permalink / raw) To: Andrii Anisov, xen-devel Cc: Andre Przywara, Stefano Stabellini, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:28 ` Andre Przywara 0 siblings, 0 replies; 27+ messages in thread From: Andre Przywara @ 2019-05-31 17:28 UTC (permalink / raw) To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:28 ` Andre Przywara 0 siblings, 0 replies; 27+ messages in thread From: Andre Przywara @ 2019-05-31 17:28 UTC (permalink / raw) To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:25 ` Andre Przywara 0 siblings, 0 replies; 27+ messages in thread From: Andre Przywara @ 2019-05-31 17:25 UTC (permalink / raw) To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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, > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:25 ` Andre Przywara 0 siblings, 0 replies; 27+ messages in thread From: Andre Przywara @ 2019-05-31 17:25 UTC (permalink / raw) To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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, > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:54 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:54 UTC (permalink / raw) To: Andre Przywara Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v2] gic: drop interrupts enabling on interrupts processing @ 2019-05-31 17:54 ` Julien Grall 0 siblings, 0 replies; 27+ messages in thread From: Julien Grall @ 2019-05-31 17:54 UTC (permalink / raw) To: Andre Przywara Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Andrii Anisov 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-06-11 7:38 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-27 9:29 [PATCH v2] gic: drop interrupts enabling on interrupts processing Andrii Anisov 2019-05-27 9:29 ` [Xen-devel] " Andrii Anisov 2019-05-28 17:07 ` Julien Grall 2019-05-28 17:07 ` [Xen-devel] " Julien Grall 2019-05-29 10:31 ` Andrii Anisov 2019-05-29 10:31 ` [Xen-devel] " Andrii Anisov 2019-05-29 15:32 ` Julien Grall 2019-05-29 15:32 ` [Xen-devel] " Julien Grall 2019-05-30 16:12 ` Andrii Anisov 2019-05-30 16:12 ` [Xen-devel] " Andrii Anisov 2019-05-31 17:11 ` Julien Grall 2019-05-31 17:11 ` [Xen-devel] " Julien Grall 2019-05-31 20:08 ` Stefano Stabellini 2019-05-31 20:08 ` [Xen-devel] " Stefano Stabellini 2019-06-10 15:49 ` Andrii Anisov 2019-06-10 19:51 ` Julien Grall 2019-06-11 7:38 ` Andrii Anisov 2019-05-30 16:14 ` Andrii Anisov 2019-05-30 16:14 ` [Xen-devel] " Andrii Anisov 2019-05-31 17:16 ` Julien Grall 2019-05-31 17:16 ` [Xen-devel] " Julien Grall 2019-05-31 17:28 ` Andre Przywara 2019-05-31 17:28 ` [Xen-devel] " Andre Przywara 2019-05-31 17:25 ` Andre Przywara 2019-05-31 17:25 ` [Xen-devel] " Andre Przywara 2019-05-31 17:54 ` Julien Grall 2019-05-31 17:54 ` [Xen-devel] " Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.