On 03/12/2018 14:46, Andre Przywara wrote: > On 30/11/2018 19:52, Andrii Anisov wrote: >> Hello Andre, >> >> Please see my comments below: >> >> On 23.11.18 14:18, Andre Przywara wrote: >>> Fundamentally there is a semantic difference between edge and level >>> triggered IRQs: When the guest has handled an *edge* IRQ (EOIed so >>> the LR's state goes to 0), this is done and dusted, and Xen doesn't >>> need to care about this anymore until the next IRQ occurs.> For level >>> triggered IRQs, even though the guest has handled it, we need to >>> resample the (potentially virtual) IRQ line, as it may come up or >>> down at the *device*'s discretion: the interrupt reason might have >>> gone away (GPIO condition no longer true), even before we were able >>> to inject it, or there might be another interrupt reason not yet >>> handled (incoming serial character while serving a transmit >>> interrupt). Also typically it's up to the interrupt handler to >>> confirm handling the interrupt, either explicitly by clearing an >>> interrupt bit in some status register or implicitly, for instance by >>> draining a FIFO, say on a serial device. So even though from the >>> (V)GIC's point of view the interrupt has been processed (EOIed), it >>> might still be pending. >> So, as I understand the intended behavior of a vGIC for the level >> interrupt is following cases: >> 1. in case the interrupt line is still active from HW side, but >> interrupt handler from VM EOIs the interrupt, it should >> be signaled to vCPU by vGIC again > > yes > >> 2. in case a peripheral deactivated interrupt line, but VM did not >> activated it yet, this interrupt should be removed from pending for >> VM > > yes > >> IMO, case 1 is indirectly supported by old vgic. For HW interrupts its >> pretty natural: deactivation by VM in VGIC leads to deactivation in >> GIC, so the interrupt priority is restored and GIC will trap PCPU to >> reinsert it. This will be seen by VM as immediate IRQ trap after EOI. > > Yes, this is true for hardware interrupts, and this lets the old VGIC > get away with it. > Virtual devices with level interrupt semantics are a different story, > the SBSA UART has some hacks in it to support it properly. > >> Also Case 2 is not implemented in the old vgic. It is somehow >> supported by new vgic, though it also relies on the trap to the >> hypervisor to commit the update to LRs. > > Yes, and there is not so much we can do about it. But that's not a real > problem, as you have this problem in bare metal, too. > >> But it's rather a problem of >> GIC arch/implementation, which does not signal CPU nor updates >> associated LR on level interrupt deassertion. >> >>> My intimate "old Xen VGIC" knowledge has been swapped out from my >>> brain meanwhile, but IIRC Xen treats every IRQ as if it would be an >>> edge IRQ. Which works if the guest's interrupt handler behaves >>> correctly. Most IRQ handlers have a loop to iterate over all possible >>> interrupt reasons and process them, so the line goes indeed down >>> before they EOI the IRQ. >> >> I've spent some time to look through the new vgic implementation and I >> have a note about it: >> It's not clear why are you probing level interrupts on guest->hyp >> transition. While it targets case 2 described above, it seems to be >> more relevant to probe the level interrupts right before hyp->guest >> transition. Because vcpu might be descheduled and while it hangs on >> scheduler queues interrupt line level has more chances to be changed >> by peripheral itself. >> Also I'm pretty scared of new vgic locking scheme with per-irq locks >> and locking logic i.e. in vgic_queue_irq_unlock() function. > > Well, you should be scared of the old VGIC locking scheme instead ;-) > Apart from the vgic_queue_irq_unlock() function, the rest of the new > locking scheme is much clearer. And it scales much better, as we have > per-IRQ locks, which should virtually never be contended, and per-CPU > locks, which are very rarely contended only, as it's mostly only taken > by its own VCPU during entry and exit. There is no per-domain lock for > the emulation anymore. > I see that it's tempting to have an "easy" locking scheme, but that > typically is either one-lock-to-rule-them-all, which doesn't scale, or > doesn't cover corner cases. > You should always prefer correctness over performance, otherwise you > just fail faster ;-) > >> Also sorting >> list in vgic_flush_lr_state() with vgic_irq_cmp() looks very >> expensive. > > Yes, but effectively this virtually never happens, as you have rarely > more than four pending IRQs at the same time. > I had patches lying around to improve this part, just never got around > to post, especially with only little rationale. > If you are interested, I can dig them out, though I am not sure how > relevant this is. > >> But, for sure all that stuff performance should be properly evaluated >> and measured. > > Indeed. Can you hack something into Xen to get some statistics on those > cases? I am not sure if Xen has something to trace lock contention, but > you could easily add some counters to track how many LRs we actually > use, to see if this is actually a problem in your case. > I think PERFCOUNTER is your friend.
CONFIG_LOCK_PROFILE and xen-lockprof on tools side? Not sure it is still working, though. Its about 9 years since I wrote and used it. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
