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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/05/2013 11:18, Gleb Natapov ha scritto:
> On Fri, May 31, 2013 at 10:48:32AM +0200, Paolo Bonzini wrote:
>> Il 31/05/2013 06:36, Gleb Natapov ha scritto:
>>> In my commit message there is two INITs in a row:
>>>  vpu0:                            vcpu1:
>>>  set INIT
>>>                                 test_and_clear_bit(KVM_APIC_INIT)
>>>                                    process INIT
>>>  set INIT
>>>  set SIPI
>>>                                 test_and_clear_bit(KVM_APIC_SIPI)
>>>                                    process SIPI
>>>
>>> Two INITs before SIPI are essential to trigger the bug
>>
>> I see now.  Let's draw pending_events as well:
>>
>>     event sent           event processed            pending_events
>>       INIT                                                INIT
>>                                INIT                        0
>>       INIT                                                INIT
>>       SIPI                                              INIT|SIPI
>>                                SIPI                       INIT
>>                                INIT                         0
>>
>> Events are reordered, there is indeed a bug if the second INIT comes at
>> just the right time.  With your patch:
>>
>>     event sent           event processed            pending_events
>>       INIT                                                INIT
>>                                INIT                        0
>>       INIT                                                INIT
>>       SIPI                                              INIT|SIPI
>>                           SIPI, failed cmpxchg          INIT|SIPI
>>                                INIT                       SIPI
>>                                SIPI                       SIPI
>
> This is incorrect. cmpxchg will fail only if another INIT cames after SIPI.
> Why  would it fail?

You're right.

Can you show what is the case in my patch where you have coalescing?  I
still prefer it because it is a smaller change, it keeps the "clear a
bit before processing" idea that you find almost everywhere.  Changing
it to "clear a bit after processing" is a bigger and more surprising
change, though both are indeed tricky.

Paolo