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

[Gleb Natapov <gleb@redhat.com>]

On Thu, May 30, 2013 at 04:15:35PM +0200, Paolo Bonzini wrote:
> 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?
> 
I did, in commit message.

> What I drew above is based on the commit message.  Instead what I
> understand from this explanation is:
> 
It is definitely not based on my commit message :)

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 and
coincidentally this is what spec advices to do.

>    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.
> 
I do not get what you are trying to tell with this. The scenario you are
repeatedly describing works with your path, with my patch and without any
patch at all.

--
			Gleb.