Il 30/05/2013 15:35, Gleb Natapov ha scritto: > 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. Maybe. >> 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. Can you draw the events that happen? What I drew above is based on the commit message. Instead what I understand from this explanation is: event sent event processed pending_events INIT INIT SIPI INIT|SIPI INIT SIPI SIPI 0 INIT INIT SIPI INIT|SIPI INIT SIPI SIPI 0 Then my patch has absolutely no effect, the cmpxchg succeeds. With your patch instead I get: event sent event processed pending_events INIT INIT SIPI INIT|SIPI INIT SIPI SIPI ... INIT INIT SIPI INIT|SIPI failed cmpxchg INIT|SIPI INIT SIPI SIPI ... successful cmpxchg 0 But there is no difference in the actual set of events that was processed. Paolo -- 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
