Re: [PATCH 2/3] KVM: Centralize slots_lock aquisition during KVM_CREATE_IRQCHIP

[Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>]

(2010/12/16 18:11), Avi Kivity wrote:
> On 12/15/2010 06:43 PM, Takuya Yoshikawa wrote:
>> From: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>
>> 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