kvm: caching API for interrupts

PATCH In review Michael S. Tsirkin 4 participants 6 messages Jul 29, 2012 – Jul 30, 2012 KVM

Share & tools

Michael S. Tsirkin

author

2012-07-29 20:00 UTC #0 lore raw

PATCH 1/1 kvm: caching API for interrupts

I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).

For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.

For now this just declares the structure and updates the APIs so it's not intended to be applied, but just to give you the idea.

Comments, flames wellcome.

Signed-off-by Michael S. Tsirkin <[email protected]>

include/linux/kvm_host.h +22 -2

index 96c158a..3384b39 100644
@@ -214,11 +214,28 @@ static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memsl
 	return ALIGN(memslot->npages, BITS_PER_LONG) / 8;
 }
 
+struct kvm_irq_cache {
+	spinlock_t lock;
+	bool valid;
+	struct kvm_vcpu *dest;
+	/* For now we only cache lapic irqs */
+	struct kvm_lapic_irq irq;
+	/* Protected by kvm->irq_cache_lock */
+	struct list_head list;
+};
+
+int kvm_set_irq_and_cache(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+			  struct kvm_irq_cache *cache);
+int kvm_set_irq_cached(struct kvm *kvm, struct kvm_irq_cache *cache);
+void kvm_irq_cache_init(struct kvm *kvm, struct kvm_irq_cache *cache);
+void kvm_irq_cache_cleanup(struct kvm_irq_cache *cache);
+
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
 	u32 type;
 	int (*set)(struct kvm_kernel_irq_routing_entry *e,
-		   struct kvm *kvm, int irq_source_id, int level);
+		   struct kvm *kvm, int irq_source_id, int level,
+		   struct kvm_irq_cache *cache);
 	union {
 		struct {
 			unsigned irqchip;

@@ -304,6 +321,8 @@ struct kvm {
 	struct kvm_irq_routing_table __rcu *irq_routing;
 	struct hlist_head mask_notifier_list;
 	struct hlist_head irq_ack_notifier_list;
+	struct mutex irq_cache_lock;
+	struct list_head irq_cache_list;
 #endif
 
 #ifdef KVM_ARCH_WANT_MMU_NOTIFIER

@@ -617,7 +636,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
-		int irq_source_id, int level);
+		int irq_source_id, int level,
+		struct kvm_irq_cache *cache);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

virt/kvm/eventfd.c +1 -1

index 7d7e2aa..9622076 100644
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 		irq = rcu_dereference(irqfd->irq_entry);
 		/* An event has been signaled, inject an interrupt */
 		if (irq)
-			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, NULL);
 		else
 			schedule_work(&irqfd->inject);
 		rcu_read_unlock();

virt/kvm/irq_comm.c +8 -5

index 5afb431..d92f3d3 100644
@@ -46,7 +46,8 @@ static inline int kvm_irq_line_state(unsigned long *irq_state,
 }
 
 static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
-			   struct kvm *kvm, int irq_source_id, int level)
+			   struct kvm *kvm, int irq_source_id, int level,
+			   struct kvm_irq_cache *cache)
 {
 #ifdef CONFIG_X86
 	struct kvm_pic *pic = pic_irqchip(kvm);

@@ -59,7 +60,8 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 }
 
 static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
-			      struct kvm *kvm, int irq_source_id, int level)
+			      struct kvm *kvm, int irq_source_id, int level,
+			      struct kvm_irq_cache *cache)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 	level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],

@@ -115,7 +117,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 }
 
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		struct kvm *kvm, int irq_source_id, int level)
+		struct kvm *kvm, int irq_source_id, int level,
+		struct kvm_irq_cache *cache)
 {
 	struct kvm_lapic_irq irq;

@@ -149,7 +152,7 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 	route.msi.address_hi = msi->address_hi;
 	route.msi.data = msi->data;
 
-	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, NULL);
 }
 
 /*

@@ -180,7 +183,7 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
 
 	while(i--) {
 		int r;
-		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level);
+		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level, NULL);
 		if (r < 0)
 			continue;

Paolo Bonzini

maintainer

2012-07-29 20:03 UTC #1 lore raw

Il 29/07/2012 22:00, Michael S. Tsirkin ha scritto:

Quoted 8 lines

I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).
For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.

seqlock?

Paolo

Michael S. Tsirkin

author

2012-07-29 20:28 UTC #2 lore raw

Quoted 6 lines · Paolo Bonzini

Il 29/07/2012 22:00, Michael S. Tsirkin ha scritto:
Quoted 5 lines
I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).

Quoted 7 lines

Quoted 3 lines
For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.
seqlock?
Paolo

AFAIK seqlock only works if uses of stale data have no side-effects, this is not the case here. We could use an rwlock I think.

Signature · 1 line

MST

Paolo Bonzini

maintainer

2012-07-30 06:57 UTC #3 lore raw

Il 29/07/2012 22:29, Michael S. Tsirkin ha scritto:

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines
Il 29/07/2012 22:00, Michael S. Tsirkin ha scritto:
Quoted 5 lines
I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.
seqlock?
AFAIK seqlock only works if uses of stale data have no side-effects, this is not the case here. We could use an rwlock I think.

If you can inject an interrupt using stale data (I think so, that would be a race in the guest between setting the route and getting the interrupt) you can use seqlock to get a consistent copy of the entry:

struct kvm_irq_cache ent, *list;

  rcu_read_lock();
  ...
  do {
    seq = read_seqcount_begin(&list->cnt);
    ent = *list;
  } while (!read_seqcount_retry(&list->cnt, seq));
  rcu_read_unlock();

Paolo

Gleb Natapov

2012-07-30 07:09 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 4 lines · Michael S. Tsirkin

I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).

I do not get this. Cache should be fill out at the time of use.

Quoted 4 lines

For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.

Cache should be invalidated at the time of use if irq routing changed.

Quoted 7 lines

For now this just declares the structure and updates the APIs so it's not intended to be applied, but just to give you the idea.
Comments, flames wellcome.

There is no enough meat to discuss here I am afraid.

Quoted 10 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

include/linux/kvm_host.h +0 -0

index 96c158a..3384b39 100644
@@ -214,11 +214,28 @@ static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memsl
 	return ALIGN(memslot->npages, BITS_PER_LONG) / 8;

include/linux/kvm_host.h static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memsl · line 224

Quoted 10 lines

include/linux/kvm_host.h +8 -0

 }
 
+struct kvm_irq_cache {
+	spinlock_t lock;
+	bool valid;
+	struct kvm_vcpu *dest;
+	/* For now we only cache lapic irqs */
+	struct kvm_lapic_irq irq;
+	/* Protected by kvm->irq_cache_lock */
+	struct list_head list;

Why are you storing them in the list? This should be embedded structure.

Quoted 103 lines

include/linux/kvm_host.h +14 -2

+};
+
+int kvm_set_irq_and_cache(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+			  struct kvm_irq_cache *cache);
+int kvm_set_irq_cached(struct kvm *kvm, struct kvm_irq_cache *cache);
+void kvm_irq_cache_init(struct kvm *kvm, struct kvm_irq_cache *cache);
+void kvm_irq_cache_cleanup(struct kvm_irq_cache *cache);
+
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
 	u32 type;
 	int (*set)(struct kvm_kernel_irq_routing_entry *e,
-		   struct kvm *kvm, int irq_source_id, int level);
+		   struct kvm *kvm, int irq_source_id, int level,
+		   struct kvm_irq_cache *cache);
 	union {
 		struct {
 			unsigned irqchip;

@@ -304,6 +321,8 @@ struct kvm {
 	struct kvm_irq_routing_table __rcu *irq_routing;
 	struct hlist_head mask_notifier_list;
 	struct hlist_head irq_ack_notifier_list;
+	struct mutex irq_cache_lock;
+	struct list_head irq_cache_list;
 #endif
 
 #ifdef KVM_ARCH_WANT_MMU_NOTIFIER

@@ -617,7 +636,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
-		int irq_source_id, int level);
+		int irq_source_id, int level,
+		struct kvm_irq_cache *cache);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

virt/kvm/eventfd.c +1 -1

index 7d7e2aa..9622076 100644
@@ -138,7 +138,7 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 		irq = rcu_dereference(irqfd->irq_entry);
 		/* An event has been signaled, inject an interrupt */
 		if (irq)
-			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, NULL);
 		else
 			schedule_work(&irqfd->inject);
 		rcu_read_unlock();

virt/kvm/irq_comm.c +8 -6

index 5afb431..d92f3d3 100644
@@ -46,7 +46,8 @@ static inline int kvm_irq_line_state(unsigned long *irq_state,
 }
 
 static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
-			   struct kvm *kvm, int irq_source_id, int level)
+			   struct kvm *kvm, int irq_source_id, int level,
+			   struct kvm_irq_cache *cache)
 {
 #ifdef CONFIG_X86
 	struct kvm_pic *pic = pic_irqchip(kvm);

@@ -59,7 +60,8 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 }
 
 static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
-			      struct kvm *kvm, int irq_source_id, int level)
+			      struct kvm *kvm, int irq_source_id, int level,
+			      struct kvm_irq_cache *cache)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 	level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],

@@ -115,7 +117,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 }
 
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		struct kvm *kvm, int irq_source_id, int level)
+		struct kvm *kvm, int irq_source_id, int level,
+		struct kvm_irq_cache *cache)
 {
 	struct kvm_lapic_irq irq;

@@ -149,7 +152,7 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 	route.msi.address_hi = msi->address_hi;
 	route.msi.data = msi->data;
 
-	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, NULL);
 }
 
 /*

@@ -180,7 +183,7 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
 
 	while(i--) {
 		int r;
-		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level);
+		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level, NULL);
 		if (r < 0)
 			continue;
 
--

Quoted 3 lines

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

--
Gleb.

Avi Kivity

2012-07-30 08:34 UTC #5 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

I've been looking at adding caching for IRQs so that we don't need to scan all VCPUs on each interrupt. One issue I had a problem with, is how the cache structure can be used from both a thread (to fill out the cache) and interrupt (to actually send if cache is valid).
For now just added a lock field in the cache so we don't need to worry about this, and with such a lock in place we don't have to worry about RCU as cache can be invalidated simply under this lock.

Quoted 5 lines

For now this just declares the structure and updates the APIs so it's not intended to be applied, but just to give you the idea.
Comments, flames wellcome.

I hope you aren't expecting any of the latter.

include/linux/kvm_host.h static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memsl · line 226

Quoted 11 lines

include/linux/kvm_host.h +10 -0

+struct kvm_irq_cache {
+	spinlock_t lock;
+	bool valid;
+	struct kvm_vcpu *dest;
+	/* For now we only cache lapic irqs */
+	struct kvm_lapic_irq irq;
+	/* Protected by kvm->irq_cache_lock */
+	struct list_head list;
+};
+

Why an external structure?

Why not add something to kvm_kernel_irq_routing_entry? It is already protected by rcu.

The atomic context code could look like this:

  if (kire->cache.valid) {
      kvm_apic_set_irq(kire->cache.vcpu, &kire->cache.irq);
      return;
  }
  queue_work(process_irq_slowpath);

The slow path tries to fills the cache, and processes the interrupt in any case.

Signature · 1 line

error compiling committee.c: too many arguments to function