From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [RFC] KVM: x86: conditionally acquire/release slots_lock on entry/exit Date: Fri, 28 Aug 2009 09:50:36 +0300 Message-ID: <4A977E3C.7050604@redhat.com> References: <20090827012000.762063112@localhost.localdomain> <20090827155450.GA6312@amt.cnet> <4A96B404.5010500@redhat.com> <20090827225940.GA13571@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Gleb Natapov To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51493 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751889AbZH1GuJ (ORCPT ); Fri, 28 Aug 2009 02:50:09 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n7S6oBrT007465 for ; Fri, 28 Aug 2009 02:50:11 -0400 In-Reply-To: <20090827225940.GA13571@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 08/28/2009 01:59 AM, Marcelo Tosatti wrote: > On Thu, Aug 27, 2009 at 07:27:48PM +0300, Avi Kivity wrote: > >> On 08/27/2009 06:54 PM, Marcelo Tosatti wrote: >> >>> perf report shows heavy overhead from down/up of slots_lock. >>> >>> Attempted to remove slots_lock by having vcpus stop on a synchronization >>> point, but this introduced further complexity (a vcpu can be scheduled >>> out before reaching the synchronization point, and can sched back in at >>> points which are slots_lock protected, etc). >>> >>> This patch changes vcpu_enter_guest to conditionally release/acquire >>> slots_lock in case a vcpu state bit is set. >>> >>> vmexit performance improves by 5-10% on UP guest. >>> >>> >> Sorry, it looks pretty complex. >> > Why? > There's a new locking protocol in there. I prefer sticking with the existing kernel plumbing, or it gets more and more difficult knowing who protects what and in what order you can do things. >> Have you considered using srcu? It seems to me down/up_read() can >> be replaced by srcu_read_lock/unlock(), and after proper conversion >> of memslots and io_bus to rcu_assign_pointer(), we can just add >> synchronize_srcu() immediately after changing stuff (of course >> mmu_lock still needs to be held when updating slots). >> > I don't see RCU as being suitable because in certain operations you > want to stop writers (on behalf of vcpus), do something, and let them > continue afterwards. The dirty log, for example. Or any operation that > wants to modify lockless vcpu specific data. > kvm_flush_remote_tlbs() (which you'd call after mmu operations), will get cpus out of guest mode, and synchronize_srcu() will wait for them to drop the srcu "read lock". So it really happens naturally: do an RCU update, send some request to all vcpus, synchronize_srcu(), done. > Also, synchronize_srcu() is limited to preemptible sections. > > io_bus could use RCU, but I think being able to stop vcpus is also a > different requirement. Do you have any suggestion on how to do it in a > better way? > We don't need to stop vcpus, just kick them out of guest mode to let them notice the new data. SRCU does that well. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.