On Thu, May 30, 2013 at 03:23:35PM +0200, Paolo Bonzini wrote: > Il 30/05/2013 15:10, Gleb Natapov ha scritto: > > On Thu, May 30, 2013 at 02:58:09PM +0200, Paolo Bonzini wrote: > >> Il 30/05/2013 14:34, Gleb Natapov ha scritto: > >>>>>>> > >>>>>>> Ah, we check kvm_apic_has_events() in runnable. Then yes, we will not > >>>>>>> lose the event. > >>>>> > >>>>> Ok, then I'd prefer to have the cmpxchg directly in the if, as in > >>>>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/110505 > >>>>> > >>> I still do not. Both of them are tricky, mine does not coalesce events > >>> needlessly. > >> > >> Agreed that both are tricky, but I don't think my patch is coalescing > >> events. If you have > >> > >> INIT SIPI INIT SIPI > >> ^ ^ > >> INIT bit cleared here SIPI bit checked here > >> > > Not sure I understand what you are trying to say here. > > I'll redo the picture below. > > >> my patch KVM sees apic_events = INIT | SIPI and deduces that the SIPI > >> bit was set by the second SIPI, not by the first. In fact the first > >> SIPI was cancelled by the second INIT, and thus should not be processed > >> at all. > > That is called coalesced. > > Coalescing would be something like INIT SIPI SIPI -> INIT SIPI. This is > not coalescing, it is proper detection of a cancelled SIPI. We have: > > event sent event processed pending_events > INIT INIT > SIPI INIT|SIPI > INIT SIPI > XX INIT INIT > SIPI INIT|SIPI > YY SIPI INIT|SIPI > failed cmpxchg INIT|SIPI > INIT SIPI > SIPI 0 > At this point I am not even sure that you understand what problem the patch is fixing, because the bug is not event triggered by above sequence. > At the line I marked with YY, you're processing an interrupt that is not > anymore in the pending_events. It was dropped at point XX. > > With my patch it is: > > > event sent event processed pending_events > INIT INIT > SIPI INIT|SIPI > INIT SIPI > XX INIT INIT > SIPI INIT|SIPI > failed cmpxchg INIT|SIPI > INIT SIPI > SIPI 0 > > >> Instead, with your patch KVM will service all four events; strictly > >> speaking it is wrong to service the first SIPI, which is why I prefer > >> having the cmpxchg in the beginning. > > > > Why is it wrong? > > Because the first SIPI was dropped atomically with the triggering of the > second INIT, it's as if you were handling it twice. > No, you were slow to process first SIPI, so second INIT was sent because vcpu appears to be dead, so instead of processing both you process last. This is exactly what coalescing means, you are just trying to present is as something desirable. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
