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: Fri, 31 May 2013 11:48:10 +0200 Message-ID: <51A871DA.7070905@redhat.com> References: <20130530070906.GA29815@redhat.com> <51A70021.1040809@redhat.com> <20130530123454.GA4845@redhat.com> <51A74CE1.1000700@redhat.com> <20130530131054.GB5495@redhat.com> <51A752D7.1020809@redhat.com> <20130530133534.GC5495@redhat.com> <51A75F07.805@redhat.com> <20130531043643.GA26250@redhat.com> <51A863E0.9080202@redhat.com> <20130531091835.GA467@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]:40481 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750719Ab3EaJsY (ORCPT ); Fri, 31 May 2013 05:48:24 -0400 In-Reply-To: <20130531091835.GA467@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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