From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v2 2/3] KVM: Optimize vcpu->requests slow path slightly Date: Thu, 31 May 2012 12:27:09 +0300 Message-ID: <4FC7396D.7050803@redhat.com> References: <1337521768-14182-1-git-send-email-avi@redhat.com> <1337521768-14182-3-git-send-email-avi@redhat.com> <20120530200334.GA23297@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:56140 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752066Ab2EaJ1L (ORCPT ); Thu, 31 May 2012 05:27:11 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4V9RArn029040 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 31 May 2012 05:27:11 -0400 In-Reply-To: <20120530200334.GA23297@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 05/30/2012 11:03 PM, Marcelo Tosatti wrote: > On Sun, May 20, 2012 at 04:49:27PM +0300, Avi Kivity wrote: >> Instead of using a atomic operation per active request, use just one >> to get all requests at once, then check them with local ops. This >> probably isn't any faster, since simultaneous requests are rare, but >> it does reduce code size. >> >> Signed-off-by: Avi Kivity >> --- >> arch/x86/kvm/x86.c | 33 ++++++++++++++++++--------------- >> 1 file changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 953e692..c0209eb 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -5232,55 +5232,58 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >> bool req_int_win = !irqchip_in_kernel(vcpu->kvm) && >> vcpu->run->request_interrupt_window; >> bool req_immediate_exit = 0; >> + ulong reqs; >> >> if (unlikely(req_int_win)) >> kvm_make_request(KVM_REQ_EVENT, vcpu); >> >> if (vcpu->requests) { >> - if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu)) >> + reqs = xchg(&vcpu->requests, 0UL); >> + >> + if (test_bit(KVM_REQ_MMU_RELOAD, &reqs)) >> kvm_mmu_unload(vcpu); >> - if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu)) >> + if (test_bit(KVM_REQ_MIGRATE_TIMER, &reqs)) >> __kvm_migrate_timers(vcpu); >> - if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu)) { >> + if (test_bit(KVM_REQ_CLOCK_UPDATE, &reqs)) { >> r = kvm_guest_time_update(vcpu); >> if (unlikely(r)) >> goto out; >> } > > Bailing out loses requests in "reqs". Whoops, good catch. > > Caching the requests makes the following type of sequence behave strangely > > req = xchg(&vcpu->requests); > if request is set > request handler > ... > set REQ_EVENT > ... > > prepare for guest entry > vcpu->requests set > bail I don't really mind that. But I do want to reduce the overhead of a request, they're not that rare in normal workloads. How about for_each_set_bit(req, &vcpu->requests, BITS_PER_LONG) { clear_bit(bit, &vcpu->requests); r = request_handlers[bit](vcpu); if (r) goto out; } ? That makes for O(1) handling since usually we only have one request set (KVM_REQ_EVENT). We'll make that the last one to avoid the scenario above. -- error compiling committee.c: too many arguments to function