Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing

[Paolo Bonzini <pbonzini@redhat.com>]

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 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.

Paolo