From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v2 4/9] KVM: arm/arm64: replace vcpu->arch.pause with a vcpu request Date: Sat, 8 Apr 2017 01:35:33 -0700 Message-ID: <20170408083533.GA28739@lvm> References: <20170331160658.4331-1-drjones@redhat.com> <20170331160658.4331-5-drjones@redhat.com> <20170404160417.GN11752@cbox> <20170404175718.su4tjx3psyez3qee@kamzik.brq.redhat.com> <20170404190438.GF31208@cbox> <63209d09-0c45-0bb2-322b-e422f1360df7@redhat.com> <20170405070959.GA1526@cbox> <91ab3b0b-c30e-7a19-f60f-882d26388ef5@redhat.com> <20170406141443.GC27123@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: marc.zyngier@arm.com, Christoffer Dall , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Fri, Apr 07, 2017 at 07:47:29PM +0800, Paolo Bonzini wrote: > > > On 06/04/2017 22:14, Christoffer Dall wrote: > >> So it seems like there are no races after all in KVM/ARM code > > > > No races after Drew's fix has been applied to set vcpu->mode = > > IN_GUEST_MODE, before checking the pause flag, correct? (I think that's > > what the spin model below is modeling). > > Yes. All of them include the vcpu->mode = IN_GUEST_MODE assignment and > (implicitly because spin is sequentially consistent) the memory barrier. > > >> After this experiment, I think I like Drew's KVM_REQ_PAUSE more than I did > >> yesterday. However, yet another alternative is to leave pause/power_off as > >> they are, while taking some inspiration from his patch to do some cleanups: > >> > >> 1) change the "if" > >> > >> if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) || > >> vcpu->arch.power_off || vcpu->arch.pause) { > >> > >> to test kvm_requests_pending instead of pause/power_off > >> > >> 2) clear KVM_REQ_VCPU_EXIT before the other "if": > >> > >> if (vcpu->arch.power_off || vcpu->arch.pause) > >> vcpu_sleep(vcpu); > > > > I like using requests as only requests from one thread to the VCPU > > thread, and not to maintain specific state about a VCPU. > > > > The benefit of Drew's approach is that since these pieces of state are > > boolean, you can have just a single check in the critical path in the > > run loop instead of having to access multiple fields. > > I think you'd still need two checks for KVM_REQ_PAUSE/KVM_REQ_POWEROFF: > one to check whether to sleep, and one to check whether to abort the > vmentry. > > Pause and power_off could be merged into a single bitmask if necessary, too. True, a sort of flags. I think at this point, I'll let Drew decide what looks cleanest when he writes up the next revision and review those. Thanks for the thorough help and checking! -Christoffer