From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v2 2/3] KVM: Optimize vcpu->requests slow path slightly
Date: Fri, 1 Jun 2012 21:23:03 -0300
Message-ID: <20120602002303.GD5874@amt.cnet>
References: <1337521768-14182-1-git-send-email-avi@redhat.com>
 <1337521768-14182-3-git-send-email-avi@redhat.com>
 <20120530200334.GA23297@amt.cnet>
 <4FC7396D.7050803@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:48328 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S965612Ab2FBAht (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 1 Jun 2012 20:37:49 -0400
Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q520bnXG021219
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Fri, 1 Jun 2012 20:37:49 -0400
Content-Disposition: inline
In-Reply-To: <4FC7396D.7050803@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, May 31, 2012 at 12:27:09PM +0300, Avi Kivity wrote:
> >> 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.

That is better.