Move irq routing data structure to rcu locking

Commit Message

Gleb Natapov July 9, 2009, 1:13 p.m. UTC

Change it from list to array to make RCU handling simpler.

Signed-off-by: Gleb Natapov <[e-mail]>
--
			Gleb.
--
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

Avi Kivity July 12, 2009, 9:50 a.m. UTC | #1

On 07/09/2009 04:13 PM, Gleb Natapov wrote:
> Change it from list to array to make RCU handling simpler.
>
> Signed-off-by: Gleb Natapov<[e-mail]>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f54a0d3..12d8d2b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -161,7 +161,8 @@ struct kvm {
>
>   	struct mutex irq_lock;
>   #ifdef CONFIG_HAVE_KVM_IRQCHIP
> -	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
> +	struct kvm_kernel_irq_routing_entry *irq_routing;
> +	spinlock_t irq_routing_lock;
>   	struct hlist_head mask_notifier_list;
>   #endif
>    

Why the new lock?

Gleb Natapov July 12, 2009, 10:07 a.m. UTC | #2

On Sun, Jul 12, 2009 at 12:50:05PM +0300, Avi Kivity wrote:
> On 07/09/2009 04:13 PM, Gleb Natapov wrote:
>> Change it from list to array to make RCU handling simpler.
>>
>> Signed-off-by: Gleb Natapov<[e-mail]>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index f54a0d3..12d8d2b 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -161,7 +161,8 @@ struct kvm {
>>
>>   	struct mutex irq_lock;
>>   #ifdef CONFIG_HAVE_KVM_IRQCHIP
>> -	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
>> +	struct kvm_kernel_irq_routing_entry *irq_routing;
>> +	spinlock_t irq_routing_lock;
>>   	struct hlist_head mask_notifier_list;
>>   #endif
>>    
>
> Why the new lock?
>
Currently routing table is protected by irq_lock mutex, but irq_lock is
overused. It protects too much of unrelated things and it is not clear what
its real purpose in life. I want to rid of it completely.

--
			Gleb.
--
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 July 12, 2009, 11:10 a.m. UTC | #3

On 07/12/2009 01:07 PM, Gleb Natapov wrote:
> On Sun, Jul 12, 2009 at 12:50:05PM +0300, Avi Kivity wrote:
>    
>> On 07/09/2009 04:13 PM, Gleb Natapov wrote:
>>      
>>> Change it from list to array to make RCU handling simpler.
>>>
>>> Signed-off-by: Gleb Natapov<[e-mail]>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index f54a0d3..12d8d2b 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -161,7 +161,8 @@ struct kvm {
>>>
>>>    	struct mutex irq_lock;
>>>    #ifdef CONFIG_HAVE_KVM_IRQCHIP
>>> -	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
>>> +	struct kvm_kernel_irq_routing_entry *irq_routing;
>>> +	spinlock_t irq_routing_lock;
>>>    	struct hlist_head mask_notifier_list;
>>>    #endif
>>>
>>>        
>> Why the new lock?
>>
>>      
> Currently routing table is protected by irq_lock mutex, but irq_lock is
> overused. It protects too much of unrelated things and it is not clear what
> its real purpose in life. I want to rid of it completely.
>    

Separate discussion/patch.

Patch

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f54a0d3..12d8d2b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -161,7 +161,8 @@  struct kvm {
 
 	struct mutex irq_lock;
 #ifdef CONFIG_HAVE_KVM_IRQCHIP
-	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
+	struct kvm_kernel_irq_routing_entry *irq_routing;
+	spinlock_t irq_routing_lock;
 	struct hlist_head mask_notifier_list;
 #endif
 
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 7af18b8..a87f8ac 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -148,7 +148,8 @@  int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	list_for_each_entry(e, &kvm->irq_routing, link)
+	rcu_read_lock();
+	for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
 		if (e->gsi == irq) {
 			int r = e->set(e, kvm, sig_level);
 			if (r < 0)
@@ -156,6 +157,8 @@  int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 
 			ret = r + ((ret < 0) ? 0 : ret);
 		}
+	}
+	rcu_read_unlock();
 	return ret;
 }
 
@@ -168,12 +171,15 @@  void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
 
 	trace_kvm_ack_irq(irqchip, pin);
 
-	list_for_each_entry(e, &kvm->irq_routing, link)
+	rcu_read_lock();
+	for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
 		if (e->irqchip.irqchip == irqchip &&
 		    e->irqchip.pin == pin) {
 			gsi = e->gsi;
 			break;
 		}
+	}
+	rcu_read_unlock();
 
 	hlist_for_each_entry(kian, n, &kvm->arch.irq_ack_notifier_list, link)
 		if (kian->gsi == gsi)
@@ -264,19 +270,11 @@  void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask)
 			kimn->func(kimn, mask);
 }
 
-static void __kvm_free_irq_routing(struct list_head *irq_routing)
-{
-	struct kvm_kernel_irq_routing_entry *e, *n;
-
-	list_for_each_entry_safe(e, n, irq_routing, link)
-		kfree(e);
-}
-
 void kvm_free_irq_routing(struct kvm *kvm)
 {
-	mutex_lock(&kvm->irq_lock);
-	__kvm_free_irq_routing(&kvm->irq_routing);
-	mutex_unlock(&kvm->irq_lock);
+	/* Called only during vm destruction. Nobody can use the pointer
+	   at this stage */
+	kfree(kvm->irq_routing);
 }
 
 static int setup_routing_entry(struct kvm_kernel_irq_routing_entry *e,
@@ -326,43 +324,39 @@  int kvm_set_irq_routing(struct kvm *kvm,
 			unsigned nr,
 			unsigned flags)
 {
-	struct list_head irq_list = LIST_HEAD_INIT(irq_list);
-	struct list_head tmp = LIST_HEAD_INIT(tmp);
-	struct kvm_kernel_irq_routing_entry *e = NULL;
+	struct kvm_kernel_irq_routing_entry *new, *old;
 	unsigned i;
 	int r;
 
+	/* last element is left zeroed and indicates the end of the array */
+	new = kzalloc(sizeof(*new) * (nr + 1), GFP_KERNEL);
+
+	if (!new)
+		return -ENOMEM;
+
 	for (i = 0; i < nr; ++i) {
 		r = -EINVAL;
 		if (ue->gsi >= KVM_MAX_IRQ_ROUTES)
 			goto out;
 		if (ue->flags)
 			goto out;
-		r = -ENOMEM;
-		e = kzalloc(sizeof(*e), GFP_KERNEL);
-		if (!e)
-			goto out;
-		r = setup_routing_entry(e, ue);
+		r = setup_routing_entry(new + i, ue);
 		if (r)
 			goto out;
 		++ue;
-		list_add(&e->link, &irq_list);
-		e = NULL;
 	}
 
-	mutex_lock(&kvm->irq_lock);
-	list_splice(&kvm->irq_routing, &tmp);
-	INIT_LIST_HEAD(&kvm->irq_routing);
-	list_splice(&irq_list, &kvm->irq_routing);
-	INIT_LIST_HEAD(&irq_list);
-	list_splice(&tmp, &irq_list);
-	mutex_unlock(&kvm->irq_lock);
+	spin_lock(&kvm->irq_routing_lock);
+	old = kvm->irq_routing;
+	rcu_assign_pointer(kvm->irq_routing, new);
+	spin_unlock(&kvm->irq_routing_lock);
+	synchronize_rcu();
 
 	r = 0;
+	new = old;
 
 out:
-	kfree(e);
-	__kvm_free_irq_routing(&irq_list);
+	kfree(new);
 	return r;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cf20dc1..8e3fd3f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -945,7 +945,7 @@  static struct kvm *kvm_create_vm(void)
 	if (IS_ERR(kvm))
 		goto out;
 #ifdef CONFIG_HAVE_KVM_IRQCHIP
-	INIT_LIST_HEAD(&kvm->irq_routing);
+	spin_lock_init(&kvm->irq_routing_lock);
 	INIT_HLIST_HEAD(&kvm->mask_notifier_list);
 #endif