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

[Paolo Bonzini <pbonzini@redhat.com>]

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