Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

Hello Julien,

On 27.11.18 17:13, Julien Grall wrote:
> Oversubscribing is usually a pretty bad idea if you want to have good 
> latency. There are no promise when the vCPU will get scheduled.
Yes, I know and clearly understand that. But we still have requirements.

> So can you describe how oversubscribed your platform is when doing the 
> benchmark?
On the setup a customer runs, its about 8 vCPU per 4 pCPUs. Under the 
benchmark conditions - system is quite idle.
> I think I now understand your problem. The problem is not because of 
> re-enabling the interrupt. Instead, it is because the GIC CPU priority 
> is been dropped using EOI early (via desc->handler->end()). As soon as 
> you drop the priority another interrupt with the same (or lower) 
> priority can fire.
I understand that.

> Looking at do_IRQ, we don't handle the same way guest IRQ and Xen IRQ.
Yep.

> The steps for Xen IRQ is roughly:
>      -> read_irq
>      -> local_irq_enable
>      -> do_IRQ
>         -> local_irq_enable (via spin_unlock_irq)
>         -> call handlers
>         -> local_irq_disable (via spin_lock_irq)
>         -> EOI
>         -> DIR
>      -> local_irq_disable
>
> The steps of for Guest IRQ is roughly:
>      -> read_irq
>      -> local_irq_enable
>      -> do_IRQ
>          -> EOI
So here we might have lower priority interrupt fired, getting us back to 
`hyp_irq()` to run `do_trap_irq()` again.
>          -> vgic_inject_irq
>              -> local_irq_disable  (via spin_lock_irqsave)
>              -> local_irq_enable   (via spin_lock_irqrestore)
>      -> local_irq_disable
>
> All vgic_inject_irq is pretty much running with interrupts disabled. The 
> Xen IRQ path seem to continue pointless enable/disable.
> So I think the following steps should suit you.
Well, do you state they do not suit mainline?

> Xen IRQ:
>      -> read_irq
>      -> do_IRQ
>          -> local_irq_enable (via spin_unlock_irq)
I suppose, isb() should be moved here from `do_trap_irq()` as well
>          -> call handlers
>          -> local_irq_disable (via spin_lock_irq)
>          -> EOI
>          -> DIR
> Guest IRQ:
>      -> read_irq
>      -> local_irq_enable
As I understand, the line above should not be there.
>      -> do_IRQ
>          -> EOI
>          -> vgic_inject_irq
>
> SGIs seems to be handled with IRQ disabled, so no change there. For 
> LPIs, we might want to do the same (needs some investigation).
--
Sincerely,
Andrii Anisov.

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