[0/4] move irq protection role to separate lock v2

Commit Message

Marcelo Tosatti May 21, 2009, 4:50 a.m. UTC

On Wed, May 20, 2009 at 03:48:41PM -0300, Marcelo Tosatti wrote:
> Addressing comment and covering irqfd (did not remove the deadlock avoidance 
> there since it might be useful later).

I can't see any reason why serializing the vm ioctl is a bad idea.

There is one indication of disagreement here:

http://patchwork.kernel.org/patch/5661/

"The original intent was that kvm_arch_vcpu_create() not "link in" the
vcpu to any registers. That allows most of the vcpu creation to happen
outside a lock."

But I fail to see the case where vcpu creation is a fast path (unless
you're benchmarking cpu hotplug/hotunplug).

Note there is no risk of a deadlock in case a vcpu thread attempts to
execute vm_ioctl.

Also vm_ioctl_lock will always be the first lock grabbed in the vm_ioctl
path, and not used in any other path, there's no risk of deadlock.

The reason for this is there are sites, such as KVM_CREATE_PIT, which
handle concurrency properly (with custom locking that becomes obsolete),
but some don't.


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christian Bornträger May 21, 2009, 6:55 a.m. UTC | #1

Am Donnerstag 21 Mai 2009 06:50:15 schrieb Marcelo Tosatti:
> But I fail to see the case where vcpu creation is a fast path (unless
> you're benchmarking cpu hotplug/hotunplug).

[...]

> @@ -2053,6 +2054,9 @@ static long kvm_vm_ioctl(struct file *fi
>
>  	if (kvm->mm != current->mm)
>  		return -EIO;
> +
> +	mutex_lock(&kvm->vm_ioctl_lock);
> +
>  	switch (ioctl) {
>  	case KVM_CREATE_VCPU:
>  		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
> @@ -2228,6 +2232,7 @@ static long kvm_vm_ioctl(struct file *fi
>  		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
>  	}
>  out:
> +	mutex_unlock(&kvm->vm_ioctl_lock);
>  	return r;
>  }

The thing that looks worrysome is that the s390 version of kvm_arch_vm_ioctl 
has KVM_S390_INTERRUPT. This allows userspace to inject interrupts - which 
would be serialized. The thing is, that external interrupts and I/O interrupts 
are floating - which means they can arrive on all cpus. This is somewhat of a 
fast path.
On the other hand, kvm_s390_inject_vm already takes the kvm->lock to protect 
agains hotplug. With this patch we might be able to remove the kvm->lock in 
kvm_s390_inject_vm - that would reduce the impact. 

This needs more thinking on our side.

Christian
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Avi Kivity May 21, 2009, 7:26 a.m. UTC | #2

Christian Bornträger wrote:
>> @@ -2053,6 +2054,9 @@ static long kvm_vm_ioctl(struct file *fi
>>
>>  	if (kvm->mm != current->mm)
>>  		return -EIO;
>> +
>> +	mutex_lock(&kvm->vm_ioctl_lock);
>> +
>>  	switch (ioctl) {
>>  	case KVM_CREATE_VCPU:
>>  		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
>> @@ -2228,6 +2232,7 @@ static long kvm_vm_ioctl(struct file *fi
>>  		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
>>  	}
>>  out:
>> +	mutex_unlock(&kvm->vm_ioctl_lock);
>>  	return r;
>>  }
>>     
>
> The thing that looks worrysome is that the s390 version of kvm_arch_vm_ioctl 
> has KVM_S390_INTERRUPT. This allows userspace to inject interrupts - which 
> would be serialized. The thing is, that external interrupts and I/O interrupts 
> are floating - which means they can arrive on all cpus. This is somewhat of a 
> fast path.
> On the other hand, kvm_s390_inject_vm already takes the kvm->lock to protect 
> agains hotplug. With this patch we might be able to remove the kvm->lock in 
> kvm_s390_inject_vm - that would reduce the impact. 
>
> This needs more thinking on our side.
>   

x86 actually shares the same problem.  KVM_IRQ_LINE interrupts may 
arrive at any vcpu.  Furthermore, with irqfd interrupts may be injected 
from userspace (the vm process or other processes) or from the kernel 
(assigned device, kernel virtio-net device).  So we have the same 
motivation to drop this lock and replace it by rcu for the fast paths.

Marcelo Tosatti May 21, 2009, 2:32 p.m. UTC | #3

On Thu, May 21, 2009 at 10:26:57AM +0300, Avi Kivity wrote:
> Christian Bornträger wrote:
>>> @@ -2053,6 +2054,9 @@ static long kvm_vm_ioctl(struct file *fi
>>>
>>>  	if (kvm->mm != current->mm)
>>>  		return -EIO;
>>> +
>>> +	mutex_lock(&kvm->vm_ioctl_lock);
>>> +
>>>  	switch (ioctl) {
>>>  	case KVM_CREATE_VCPU:
>>>  		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
>>> @@ -2228,6 +2232,7 @@ static long kvm_vm_ioctl(struct file *fi
>>>  		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
>>>  	}
>>>  out:
>>> +	mutex_unlock(&kvm->vm_ioctl_lock);
>>>  	return r;
>>>  }
>>>     
>>
>> The thing that looks worrysome is that the s390 version of 
>> kvm_arch_vm_ioctl has KVM_S390_INTERRUPT. This allows userspace to 
>> inject interrupts - which would be serialized. The thing is, that 
>> external interrupts and I/O interrupts are floating - which means they 
>> can arrive on all cpus. This is somewhat of a fast path.
>> On the other hand, kvm_s390_inject_vm already takes the kvm->lock to 
>> protect agains hotplug. With this patch we might be able to remove the 
>> kvm->lock in kvm_s390_inject_vm - that would reduce the impact. 
>>
>> This needs more thinking on our side.
>>   
>
> x86 actually shares the same problem.  KVM_IRQ_LINE interrupts may  
> arrive at any vcpu.  Furthermore, with irqfd interrupts may be injected  
> from userspace (the vm process or other processes) or from the kernel  
> (assigned device, kernel virtio-net device).  So we have the same  
> motivation to drop this lock and replace it by rcu for the fast paths.

OK, will use the lock to serialize individual ioctl commands that are
not performance sensitive and need serialization.

Any objection to v2 of the irq_lock patch?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Avi Kivity May 21, 2009, 3:02 p.m. UTC | #4

Marcelo Tosatti wrote:

  

>> x86 actually shares the same problem.  KVM_IRQ_LINE interrupts may  
>> arrive at any vcpu.  Furthermore, with irqfd interrupts may be injected  
>> from userspace (the vm process or other processes) or from the kernel  
>> (assigned device, kernel virtio-net device).  So we have the same  
>> motivation to drop this lock and replace it by rcu for the fast paths.
>>     
>
> OK, will use the lock to serialize individual ioctl commands that are
> not performance sensitive and need serialization.
>   

Locks should protect data, not operations.  Something named ioctl_lock 
indicates something is wrong.

> Any objection to v2 of the irq_lock patch?
>   

Haven't done a detailed review yet, sorry.

Patch

Index: kvm/include/linux/kvm_host.h
===================================================================
--- kvm.orig/include/linux/kvm_host.h
+++ kvm/include/linux/kvm_host.h
@@ -131,9 +131,9 @@  struct kvm {
 					KVM_PRIVATE_MEM_SLOTS];
 	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
 	struct list_head vm_list;
+	struct mutex vm_ioctl_lock; /* protects concurrent vm fd ioctls */
 	struct mutex lock; /*
 			    * - protects mmio_bus, pio_bus.
-			    * - protects a few concurrent ioctl's (FIXME).
 			    * - protects concurrent create_vcpu, but
 			    *   kvm->vcpus walkers do it locklessly (FIXME).
 			    */
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -988,6 +988,7 @@  static struct kvm *kvm_create_vm(void)
 	INIT_LIST_HEAD(&kvm->irqfds);
 	mutex_init(&kvm->lock);
 	mutex_init(&kvm->irq_lock);
+	mutex_init(&kvm->vm_ioctl_lock);
 	kvm_io_bus_init(&kvm->mmio_bus);
 	init_rwsem(&kvm->slots_lock);
 	atomic_set(&kvm->users_count, 1);
@@ -2053,6 +2054,9 @@  static long kvm_vm_ioctl(struct file *fi
 
 	if (kvm->mm != current->mm)
 		return -EIO;
+
+	mutex_lock(&kvm->vm_ioctl_lock);
+
 	switch (ioctl) {
 	case KVM_CREATE_VCPU:
 		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
@@ -2228,6 +2232,7 @@  static long kvm_vm_ioctl(struct file *fi
 		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
 	}
 out:
+	mutex_unlock(&kvm->vm_ioctl_lock);
 	return r;
 }