From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Subject: Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP Date: Thu, 16 Dec 2010 18:29:31 +0900 Message-ID: <4D09DBFB.1080103@oss.ntt.co.jp> References: <20101216013917.a8a8bab9.takuya.yoshikawa@gmail.com> <20101216014326.9210211a.takuya.yoshikawa@gmail.com> <4D09D7DF.3010209@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Takuya Yoshikawa , mtosatti@redhat.com, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org To: Avi Kivity Return-path: Received: from serv2.oss.ntt.co.jp ([222.151.198.100]:56950 "EHLO serv2.oss.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751441Ab0LPJ0j (ORCPT ); Thu, 16 Dec 2010 04:26:39 -0500 In-Reply-To: <4D09D7DF.3010209@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: (2010/12/16 18:11), Avi Kivity wrote: > On 12/15/2010 06:43 PM, Takuya Yoshikawa wrote: >> From: Takuya Yoshikawa >> >> Move slots_lock aquisition from kvm_ioapic_init() and kvm_create_pic() >> to their caller. >> >> As a result, x86's KVM_CREATE_IRQCHIP is now covered by a unified slots_lock >> section, including kvm_setup_default_irq_routing(). >> >> > > I'm not sure about this... > >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -3300,6 +3300,7 @@ long kvm_arch_vm_ioctl(struct file *filp, >> struct kvm_pic *vpic; >> >> mutex_lock(&kvm->lock); >> + mutex_lock(&kvm->slots_lock); > > here, the reader might wonder why we take slots_lock, since we aren't manipulating anything covered by the lock directly. > >> r = -EEXIST; >> if (kvm->arch.vpic) >> goto create_irqchip_unlock; >> @@ -3308,10 +3309,8 @@ long kvm_arch_vm_ioctl(struct file *filp, >> if (vpic) { >> r = kvm_ioapic_init(kvm); >> if (r) { >> - mutex_lock(&kvm->slots_lock); > > and here, the reader might wonder why we don't take slots_lock, which protects io_bus. > > Maybe we ought to move slots_lock acquisition to kvm_io_bus_register() and friends. > So it will move the lock acquisition to the opposite ( callee ) side than mine. At first, I tried to do that, but there are so many ... Anyway, your suggestion seems to be the best way if possible. One question: how about kvm_io_bus_[read|write] ? These are called from the emulator but I could not find where slots_lock are held though I can see the comments "kvm_io_bus_[read|write] - called under kvm->slots_lock" Takuya