Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760001AbbBIWoq (ORCPT ); Mon, 9 Feb 2015 17:44:46 -0500 Received: from mail-ie0-f170.google.com ([209.85.223.170]:41214 "EHLO mail-ie0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753941AbbBIWop (ORCPT ); Mon, 9 Feb 2015 17:44:45 -0500 MIME-Version: 1.0 In-Reply-To: <1423225019-11001-2-git-send-email-pbonzini@redhat.com> References: <1423225019-11001-1-git-send-email-pbonzini@redhat.com> <1423225019-11001-2-git-send-email-pbonzini@redhat.com> From: David Matlack Date: Mon, 9 Feb 2015 14:44:23 -0800 Message-ID: Subject: Re: [PATCH 1/2] KVM: x86: extract guest running logic from __vcpu_run To: Paolo Bonzini Cc: "linux-kernel@vger.kernel.org" , kvm list , riel@redhat.com, Marcelo Tosatti , rkrcmar@redhat.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5713 Lines: 155 On Fri, Feb 6, 2015 at 4:16 AM, Paolo Bonzini wrote: > Rename the old __vcpu_run to vcpu_run, and extract its main logic > to a new function. > > The next patch will add a new early-exit condition to __vcpu_run, > avoid extra indentation. > > Signed-off-by: Paolo Bonzini > --- Reviewed-by: David Matlack > arch/x86/kvm/x86.c | 67 +++++++++++++++++++++++++++++------------------------- > 1 file changed, 36 insertions(+), 31 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index bd7a70be41b3..0b8dd13676ef 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6165,7 +6165,7 @@ void kvm_arch_mmu_notifier_invalidate_page(struct kvm *kvm, > } > > /* > - * Returns 1 to let __vcpu_run() continue the guest execution loop without > + * Returns 1 to let vcpu_run() continue the guest execution loop without > * exiting to the userspace. Otherwise, the value will be returned to the > * userspace. > */ > @@ -6383,42 +6383,45 @@ out: > return r; > } > > +static inline int __vcpu_run(struct kvm *kvm, struct kvm_vcpu *vcpu) Kind of confusing function body given the name. Maybe put if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && !vcpu->arch.apf.halted) return vcpu_enter_guest(vcpu); back in vcpu_run and rename this function? vcpu_wait? > +{ > + if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && > + !vcpu->arch.apf.halted) > + return vcpu_enter_guest(vcpu); > + > + srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx); > + kvm_vcpu_block(vcpu); > + vcpu->srcu_idx = srcu_read_lock(&kvm->srcu); > + if (!kvm_check_request(KVM_REQ_UNHALT, vcpu)) > + return 1; > > -static int __vcpu_run(struct kvm_vcpu *vcpu) > + kvm_apic_accept_events(vcpu); > + switch(vcpu->arch.mp_state) { > + case KVM_MP_STATE_HALTED: > + vcpu->arch.pv.pv_unhalted = false; > + vcpu->arch.mp_state = > + KVM_MP_STATE_RUNNABLE; > + case KVM_MP_STATE_RUNNABLE: > + vcpu->arch.apf.halted = false; > + break; > + case KVM_MP_STATE_INIT_RECEIVED: > + break; > + default: > + return -EINTR; > + break; > + } > + return 1; > +} > + > +static int vcpu_run(struct kvm_vcpu *vcpu) vcpu_run_loop might be a clearer name. > { > int r; > struct kvm *kvm = vcpu->kvm; > > vcpu->srcu_idx = srcu_read_lock(&kvm->srcu); > > - r = 1; > - while (r > 0) { > - if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE && > - !vcpu->arch.apf.halted) > - r = vcpu_enter_guest(vcpu); > - else { > - srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx); > - kvm_vcpu_block(vcpu); > - vcpu->srcu_idx = srcu_read_lock(&kvm->srcu); > - if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { > - kvm_apic_accept_events(vcpu); > - switch(vcpu->arch.mp_state) { > - case KVM_MP_STATE_HALTED: > - vcpu->arch.pv.pv_unhalted = false; > - vcpu->arch.mp_state = > - KVM_MP_STATE_RUNNABLE; > - case KVM_MP_STATE_RUNNABLE: > - vcpu->arch.apf.halted = false; > - break; > - case KVM_MP_STATE_INIT_RECEIVED: > - break; > - default: > - r = -EINTR; > - break; > - } > - } > - } > - > + for (;;) { > + r = __vcpu_run(kvm, vcpu); > if (r <= 0) > break; > > @@ -6430,6 +6433,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) > r = -EINTR; > vcpu->run->exit_reason = KVM_EXIT_INTR; > ++vcpu->stat.request_irq_exits; > + break; > } > > kvm_check_async_pf_completion(vcpu); > @@ -6438,6 +6442,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) > r = -EINTR; > vcpu->run->exit_reason = KVM_EXIT_INTR; > ++vcpu->stat.signal_exits; > + break; The removal of the loop condition and the addition of these "break"s change the loop behavior slightly, but I think it's safe. We'll start breaking before checking need_resched(), but we're about to return to userspace anyway so we'll reschedule then. > } > if (need_resched()) { > srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx); > @@ -6569,7 +6574,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > } else > WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed); > > - r = __vcpu_run(vcpu); > + r = vcpu_run(vcpu); > > out: > post_kvm_run_save(vcpu); > -- > 1.8.3.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/