From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing Date: Thu, 30 May 2013 15:23:35 +0200 Message-ID: <51A752D7.1020809@redhat.com> References: <20130528150057.GA6891@redhat.com> <51A4DC63.2040906@redhat.com> <20130530012048.GB20766@redhat.com> <51A6E671.8070408@redhat.com> <20130530060150.GA28173@redhat.com> <51A6F22F.2000600@redhat.com> <20130530070906.GA29815@redhat.com> <51A70021.1040809@redhat.com> <20130530123454.GA4845@redhat.com> <51A74CE1.1000700@redhat.com> <20130530131054.GB5495@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Jan Kiszka To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39977 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765Ab3E3NXt (ORCPT ); Thu, 30 May 2013 09:23:49 -0400 In-Reply-To: <20130530131054.GB5495@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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