[PATCH RFC 1/2] kvm: pass host irq number to set irq calls

[PATCH RFC 1/2] kvm: pass host irq number to set irq calls

[Michael S. Tsirkin]

Pass host irq number to functions that set guest irq.
If unavailable, pass -1.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 arch/x86/kvm/i8254.c     |    4 ++--
 arch/x86/kvm/lapic.c     |    2 +-
 arch/x86/kvm/x86.c       |    2 +-
 include/linux/kvm_host.h |    7 ++++---
 virt/kvm/assigned-dev.c  |    9 +++++----
 virt/kvm/eventfd.c       |    7 ++++---
 virt/kvm/ioapic.c        |    2 +-
 virt/kvm/ioapic.h        |    2 +-
 virt/kvm/irq_comm.c      |   18 +++++++++++-------
 9 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index efad723..6b1b47b 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -303,8 +303,8 @@ static void pit_do_work(struct work_struct *work)
 	}
 	spin_unlock(&ps->inject_lock);
 	if (inject) {
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, -1);
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, -1);
 
 		/*
 		 * Provides NMI watchdog support via Virtual Wire mode.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 57dcbd4..a643a27 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -508,7 +508,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 		   irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode,
 		   irq.vector);
 
-	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq);
+	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, -1);
 }
 
 static u32 apic_get_tmcct(struct kvm_lapic *apic)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 84a28ea..8e80bf3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3610,7 +3610,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (irqchip_in_kernel(kvm)) {
 			__s32 status;
 			status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
-					irq_event.irq, irq_event.level);
+					irq_event.irq, irq_event.level, -1);
 			if (ioctl == KVM_IRQ_LINE_STATUS) {
 				r = -EFAULT;
 				irq_event.status = status;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index eabb21a..9078b44 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -192,7 +192,7 @@ 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, int host_irq);
 	union {
 		struct {
 			unsigned irqchip;
@@ -546,9 +546,10 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   union kvm_ioapic_redirect_entry *entry,
 				   unsigned long *deliver_bitmask);
 #endif
-int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+		int host_irq);
 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, int host_irq);
 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);
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 4e9eaeb..f89f138 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -74,11 +74,12 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id)
 			vector = assigned_dev->
 					guest_msix_entries[index].vector;
 			kvm_set_irq(assigned_dev->kvm,
-				    assigned_dev->irq_source_id, vector, 1);
+				    assigned_dev->irq_source_id, vector, 1,
+				    irq);
 		}
 	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    assigned_dev->guest_irq, 1);
+			    assigned_dev->guest_irq, 1, irq);
 
 	return IRQ_HANDLED;
 }
@@ -94,7 +95,7 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	dev = container_of(kian, struct kvm_assigned_dev_kernel,
 			   ack_notifier);
 
-	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0);
+	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0, -1);
 
 	/* The guest irq may be shared so this ack may be
 	 * from another device.
@@ -114,7 +115,7 @@ static void deassign_guest_irq(struct kvm *kvm,
 	assigned_dev->ack_notifier.gsi = -1;
 
 	kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-		    assigned_dev->guest_irq, 0);
+		    assigned_dev->guest_irq, 0, -1);
 
 	if (assigned_dev->irq_source_id != -1)
 		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 73358d2..8fcec75 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -67,8 +67,8 @@ irqfd_inject(struct work_struct *work)
 	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
 	struct kvm *kvm = irqfd->kvm;
 
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1, -1);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0, -1);
 }
 
 /*
@@ -138,7 +138,8 @@ 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,
+				    -1);
 		else
 			schedule_work(&irqfd->inject);
 		rcu_read_unlock();
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 8df1ca1..adaa9d8 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -188,7 +188,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 		irqe.dest_id = ioapic->kvm->bsp_vcpu->vcpu_id;
 	}
 #endif
-	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
+	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, -1);
 }
 
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 0b190c3..1cca4b0 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -76,7 +76,7 @@ void kvm_ioapic_destroy(struct kvm *kvm);
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq);
+		struct kvm_lapic_irq *irq, int host_irq);
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 9f614b4..ac8b629 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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,
+			   int host_irq)
 {
 #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,
+			      int host_irq)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 	level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
@@ -79,7 +81,7 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
 }
 
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq)
+		struct kvm_lapic_irq *irq, int host_irq)
 {
 	int i, r = -1;
 	struct kvm_vcpu *vcpu, *lowest = NULL;
@@ -115,7 +117,7 @@ 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, int host_irq)
 {
 	struct kvm_lapic_irq irq;
 
@@ -135,7 +137,7 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 	irq.shorthand = 0;
 
 	/* TODO Deal with RH bit of MSI message address */
-	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
+	return kvm_irq_delivery_to_apic(kvm, NULL, &irq, host_irq);
 }
 
 /*
@@ -144,7 +146,8 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
  *  = 0   Interrupt was coalesced (previous irq is still pending)
  *  > 0   Number of CPUs interrupt was delivered to
  */
-int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+		int host_irq)
 {
 	struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
 	int ret = -1, i = 0;
@@ -166,7 +169,8 @@ 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,
+                                   host_irq);
 		if (r < 0)
 			continue;
 
-- 
1.7.5.53.gc233e

[PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

To forward an interrupt to a vcpu that runs on
a host cpu different from the current one,
we need an ipi which likely will cost us as much
as delivering the interrupt directly to that cpu would.

Set irq affinity hint to point there, irq balancer
can then take this into accound and balance
interrupts accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index f89f138..b579777 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		for (i = 0; i < assigned_dev->entries_nr; i++)
 			disable_irq(assigned_dev->host_msix_entries[i].vector);
 
-		for (i = 0; i < assigned_dev->entries_nr; i++)
-			free_irq(assigned_dev->host_msix_entries[i].vector,
-				 (void *)assigned_dev);
+		for (i = 0; i < assigned_dev->entries_nr; i++) {
+			u32 vector = assigned_dev->host_msix_entries[i].vector;
+			irq_set_affinity_hint(vector, NULL);
+			free_irq(vector, (void *)assigned_dev);
+		}
 
 		assigned_dev->entries_nr = 0;
 		kfree(assigned_dev->host_msix_entries);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index ac8b629..68b1f7c 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -22,6 +22,7 @@
 
 #include <linux/kvm_host.h>
 #include <linux/slab.h>
+#include <linux/interrupt.h>
 #include <trace/events/kvm.h>
 
 #include <asm/msidef.h>
@@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
 #endif
 }
 
+static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
+{
+	const struct cpumask *mask;
+	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
+	 * wrong value but we don't mind much. */
+	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
+		mask = get_cpu_mask(vcpu->cpu);
+		irq_set_affinity_hint(host_irq, mask);
+	}
+}
+	
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, int host_irq)
 {
@@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			if (r < 0)
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq);
+			kvm_vcpu_host_irq_hint(vcpu, host_irq);
 		} else if (kvm_lapic_enabled(vcpu)) {
 			if (!lowest)
 				lowest = vcpu;
@@ -110,8 +123,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		}
 	}
 
-	if (lowest)
+	if (lowest) {
 		r = kvm_apic_set_irq(lowest, irq);
+		kvm_vcpu_host_irq_hint(vcpu, host_irq);
+	}
 
 	return r;
 }
-- 
1.7.5.53.gc233e

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Marcelo Tosatti]

On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> To forward an interrupt to a vcpu that runs on
> a host cpu different from the current one,
> we need an ipi which likely will cost us as much
> as delivering the interrupt directly to that cpu would.
> 
> Set irq affinity hint to point there, irq balancer
> can then take this into accound and balance
> interrupts accordingly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  virt/kvm/assigned-dev.c |    8 +++++---
>  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index f89f138..b579777 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
>  		for (i = 0; i < assigned_dev->entries_nr; i++)
>  			disable_irq(assigned_dev->host_msix_entries[i].vector);
>  
> -		for (i = 0; i < assigned_dev->entries_nr; i++)
> -			free_irq(assigned_dev->host_msix_entries[i].vector,
> -				 (void *)assigned_dev);
> +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> +			irq_set_affinity_hint(vector, NULL);
> +			free_irq(vector, (void *)assigned_dev);
> +		}
>  
>  		assigned_dev->entries_nr = 0;
>  		kfree(assigned_dev->host_msix_entries);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index ac8b629..68b1f7c 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/slab.h>
> +#include <linux/interrupt.h>
>  #include <trace/events/kvm.h>
>  
>  #include <asm/msidef.h>
> @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
>  #endif
>  }
>  
> +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> +{
> +	const struct cpumask *mask;
> +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> +	 * wrong value but we don't mind much. */
> +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> +		mask = get_cpu_mask(vcpu->cpu);
> +		irq_set_affinity_hint(host_irq, mask);
> +	}
> +}

Unsure about the internals of irq_set_affinity_hint, but AFAICS its
exported so that irqbalance in userspace can make a decision.

If that is the case, then irqbalance update rate should be high enough
to catch up with a vcpu migrating betweens cpus (which initially does
not appear a sensible arrangement).

The decision to have the host interrupt follow the vcpu seems a good
one, given that it saves an IPI and is potentially more cache friendly
overall.

And AFAICS its more intelligent for the device assignment case than
anything irqbalance can come up with (note it depends on how the APIC is
configured, your patch ignores that).

> +	
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, int host_irq)
>  {
> @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  			if (r < 0)
>  				r = 0;
>  			r += kvm_apic_set_irq(vcpu, irq);
> +			kvm_vcpu_host_irq_hint(vcpu, host_irq);
>  		} else if (kvm_lapic_enabled(vcpu)) {
>  			if (!lowest)
>  				lowest = vcpu;
> @@ -110,8 +123,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		}
>  	}
>  
> -	if (lowest)
> +	if (lowest) {
>  		r = kvm_apic_set_irq(lowest, irq);
> +		kvm_vcpu_host_irq_hint(vcpu, host_irq);
> +	}
>  
>  	return r;
>  }
> -- 
> 1.7.5.53.gc233e

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > To forward an interrupt to a vcpu that runs on
> > a host cpu different from the current one,
> > we need an ipi which likely will cost us as much
> > as delivering the interrupt directly to that cpu would.
> > 
> > Set irq affinity hint to point there, irq balancer
> > can then take this into accound and balance
> > interrupts accordingly.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  virt/kvm/assigned-dev.c |    8 +++++---
> >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > index f89f138..b579777 100644
> > --- a/virt/kvm/assigned-dev.c
> > +++ b/virt/kvm/assigned-dev.c
> > @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
> >  		for (i = 0; i < assigned_dev->entries_nr; i++)
> >  			disable_irq(assigned_dev->host_msix_entries[i].vector);
> >  
> > -		for (i = 0; i < assigned_dev->entries_nr; i++)
> > -			free_irq(assigned_dev->host_msix_entries[i].vector,
> > -				 (void *)assigned_dev);
> > +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> > +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> > +			irq_set_affinity_hint(vector, NULL);
> > +			free_irq(vector, (void *)assigned_dev);
> > +		}
> >  
> >  		assigned_dev->entries_nr = 0;
> >  		kfree(assigned_dev->host_msix_entries);
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index ac8b629..68b1f7c 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -22,6 +22,7 @@
> >  
> >  #include <linux/kvm_host.h>
> >  #include <linux/slab.h>
> > +#include <linux/interrupt.h>
> >  #include <trace/events/kvm.h>
> >  
> >  #include <asm/msidef.h>
> > @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> >  #endif
> >  }
> >  
> > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > +{
> > +	const struct cpumask *mask;
> > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > +	 * wrong value but we don't mind much. */
> > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > +		mask = get_cpu_mask(vcpu->cpu);
> > +		irq_set_affinity_hint(host_irq, mask);
> > +	}
> > +}
> 
> Unsure about the internals of irq_set_affinity_hint, but AFAICS its
> exported so that irqbalance in userspace can make a decision.

Yes. Pls note at the moment there's no hint so irqbalance
will likely try to move the irq away from vcpu if that
is doing a lot of work. My patch tries to correct that.

> If that is the case, then irqbalance update rate should be high enough
> to catch up with a vcpu migrating betweens cpus (which initially does
> not appear a sensible arrangement).

At least for pinned vcpus, that's almost sure to be the case :)

> The decision to have the host interrupt follow the vcpu seems a good
> one, given that it saves an IPI and is potentially more cache friendly
> overall.

> And AFAICS its more intelligent for the device assignment case than
> anything irqbalance can come up with

Do you just propose overwriting affinity set by userspace then?
My concern would be to avoid breaking setups some users have,
with carefully manually optimized affinity for vcpus and device irqs.

> (note it depends on how the APIC is
> configured, your patch ignores that).

Could you clarify please? What is meant by 'it' in 'it depends'?
Which APIC - host or guest - do you mean, and what are possible APIC
configurations to consider?

> >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  		struct kvm_lapic_irq *irq, int host_irq)
> >  {
> > @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  			if (r < 0)
> >  				r = 0;
> >  			r += kvm_apic_set_irq(vcpu, irq);
> > +			kvm_vcpu_host_irq_hint(vcpu, host_irq);
> >  		} else if (kvm_lapic_enabled(vcpu)) {
> >  			if (!lowest)
> >  				lowest = vcpu;
> > @@ -110,8 +123,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  		}
> >  	}
> >  
> > -	if (lowest)
> > +	if (lowest) {
> >  		r = kvm_apic_set_irq(lowest, irq);
> > +		kvm_vcpu_host_irq_hint(vcpu, host_irq);
> > +	}
> >  
> >  	return r;
> >  }
> > -- 
> > 1.7.5.53.gc233e

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Marcelo Tosatti]

On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > To forward an interrupt to a vcpu that runs on
> > > a host cpu different from the current one,
> > > we need an ipi which likely will cost us as much
> > > as delivering the interrupt directly to that cpu would.
> > > 
> > > Set irq affinity hint to point there, irq balancer
> > > can then take this into accound and balance
> > > interrupts accordingly.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  virt/kvm/assigned-dev.c |    8 +++++---
> > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > index f89f138..b579777 100644
> > > --- a/virt/kvm/assigned-dev.c
> > > +++ b/virt/kvm/assigned-dev.c
> > > @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
> > >  		for (i = 0; i < assigned_dev->entries_nr; i++)
> > >  			disable_irq(assigned_dev->host_msix_entries[i].vector);
> > >  
> > > -		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > -			free_irq(assigned_dev->host_msix_entries[i].vector,
> > > -				 (void *)assigned_dev);
> > > +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> > > +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> > > +			irq_set_affinity_hint(vector, NULL);
> > > +			free_irq(vector, (void *)assigned_dev);
> > > +		}
> > >  
> > >  		assigned_dev->entries_nr = 0;
> > >  		kfree(assigned_dev->host_msix_entries);
> > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > index ac8b629..68b1f7c 100644
> > > --- a/virt/kvm/irq_comm.c
> > > +++ b/virt/kvm/irq_comm.c
> > > @@ -22,6 +22,7 @@
> > >  
> > >  #include <linux/kvm_host.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/interrupt.h>
> > >  #include <trace/events/kvm.h>
> > >  
> > >  #include <asm/msidef.h>
> > > @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > >  #endif
> > >  }
> > >  
> > > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > > +{
> > > +	const struct cpumask *mask;
> > > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > > +	 * wrong value but we don't mind much. */
> > > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > > +		mask = get_cpu_mask(vcpu->cpu);
> > > +		irq_set_affinity_hint(host_irq, mask);
> > > +	}
> > > +}
> > 
> > Unsure about the internals of irq_set_affinity_hint, but AFAICS its
> > exported so that irqbalance in userspace can make a decision.
> 
> Yes. Pls note at the moment there's no hint so irqbalance
> will likely try to move the irq away from vcpu if that
> is doing a lot of work. My patch tries to correct that.
> 
> > If that is the case, then irqbalance update rate should be high enough
> > to catch up with a vcpu migrating betweens cpus (which initially does
> > not appear a sensible arrangement).
> 
> At least for pinned vcpus, that's almost sure to be the case :)

What i mean is that the frequency of a vcpu migrating between cpus
might be higher than what irqbalance can cope with.

> > The decision to have the host interrupt follow the vcpu seems a good
> > one, given that it saves an IPI and is potentially more cache friendly
> > overall.
> 
> > And AFAICS its more intelligent for the device assignment case than
> > anything irqbalance can come up with
> 
> Do you just propose overwriting affinity set by userspace then?

Yes.

> My concern would be to avoid breaking setups some users have,
> with carefully manually optimized affinity for vcpus and device irqs.

They can disable automatic in-kernel affinity.

> 
> > (note it depends on how the APIC is
> > configured, your patch ignores that).
> 
> Could you clarify please? What is meant by 'it' in 'it depends'?

"It" means the target vcpu selection. It depends on how the guest
APIC is programmed.

> Which APIC - host or guest - do you mean, and what are possible APIC
> configurations to consider?

Guest APIC. Guest APIC programmed with round robin would break the
static assignment on your patch.

Configurations to consider, all common ones used for assigned devices?


> > >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > >  		struct kvm_lapic_irq *irq, int host_irq)
> > >  {
> > > @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > >  			if (r < 0)
> > >  				r = 0;
> > >  			r += kvm_apic_set_irq(vcpu, irq);
> > > +			kvm_vcpu_host_irq_hint(vcpu, host_irq);
> > >  		} else if (kvm_lapic_enabled(vcpu)) {
> > >  			if (!lowest)
> > >  				lowest = vcpu;
> > > @@ -110,8 +123,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > >  		}
> > >  	}
> > >  
> > > -	if (lowest)
> > > +	if (lowest) {
> > >  		r = kvm_apic_set_irq(lowest, irq);
> > > +		kvm_vcpu_host_irq_hint(vcpu, host_irq);
> > > +	}
> > >  
> > >  	return r;
> > >  }
> > > -- 
> > > 1.7.5.53.gc233e

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Marcelo Tosatti]

On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > To forward an interrupt to a vcpu that runs on
> > > a host cpu different from the current one,
> > > we need an ipi which likely will cost us as much
> > > as delivering the interrupt directly to that cpu would.
> > > 
> > > Set irq affinity hint to point there, irq balancer
> > > can then take this into accound and balance
> > > interrupts accordingly.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  virt/kvm/assigned-dev.c |    8 +++++---
> > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > >  2 files changed, 21 insertions(+), 4 deletions(-)

Or maybe even automatic in-kernel irq affinity disabled by 
default (to avoid interference with userspace pinning). 
All of that if it makes sense performance wise, of course.

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 08:58:59AM -0200, Marcelo Tosatti wrote:
> On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > > To forward an interrupt to a vcpu that runs on
> > > > a host cpu different from the current one,
> > > > we need an ipi which likely will cost us as much
> > > > as delivering the interrupt directly to that cpu would.
> > > > 
> > > > Set irq affinity hint to point there, irq balancer
> > > > can then take this into accound and balance
> > > > interrupts accordingly.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  virt/kvm/assigned-dev.c |    8 +++++---
> > > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > index f89f138..b579777 100644
> > > > --- a/virt/kvm/assigned-dev.c
> > > > +++ b/virt/kvm/assigned-dev.c
> > > > @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
> > > >  		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > >  			disable_irq(assigned_dev->host_msix_entries[i].vector);
> > > >  
> > > > -		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > > -			free_irq(assigned_dev->host_msix_entries[i].vector,
> > > > -				 (void *)assigned_dev);
> > > > +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> > > > +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> > > > +			irq_set_affinity_hint(vector, NULL);
> > > > +			free_irq(vector, (void *)assigned_dev);
> > > > +		}
> > > >  
> > > >  		assigned_dev->entries_nr = 0;
> > > >  		kfree(assigned_dev->host_msix_entries);
> > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > index ac8b629..68b1f7c 100644
> > > > --- a/virt/kvm/irq_comm.c
> > > > +++ b/virt/kvm/irq_comm.c
> > > > @@ -22,6 +22,7 @@
> > > >  
> > > >  #include <linux/kvm_host.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/interrupt.h>
> > > >  #include <trace/events/kvm.h>
> > > >  
> > > >  #include <asm/msidef.h>
> > > > @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > >  #endif
> > > >  }
> > > >  
> > > > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > > > +{
> > > > +	const struct cpumask *mask;
> > > > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > > > +	 * wrong value but we don't mind much. */
> > > > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > > > +		mask = get_cpu_mask(vcpu->cpu);
> > > > +		irq_set_affinity_hint(host_irq, mask);
> > > > +	}
> > > > +}
> > > 
> > > Unsure about the internals of irq_set_affinity_hint, but AFAICS its
> > > exported so that irqbalance in userspace can make a decision.
> > 
> > Yes. Pls note at the moment there's no hint so irqbalance
> > will likely try to move the irq away from vcpu if that
> > is doing a lot of work. My patch tries to correct that.
> > 
> > > If that is the case, then irqbalance update rate should be high enough
> > > to catch up with a vcpu migrating betweens cpus (which initially does
> > > not appear a sensible arrangement).
> > 
> > At least for pinned vcpus, that's almost sure to be the case :)
> 
> What i mean is that the frequency of a vcpu migrating between cpus
> might be higher than what irqbalance can cope with.
> 
> > > The decision to have the host interrupt follow the vcpu seems a good
> > > one, given that it saves an IPI and is potentially more cache friendly
> > > overall.
> > 
> > > And AFAICS its more intelligent for the device assignment case than
> > > anything irqbalance can come up with
> > 
> > Do you just propose overwriting affinity set by userspace then?
> 
> Yes.
> 
> > My concern would be to avoid breaking setups some users have,
> > with carefully manually optimized affinity for vcpus and device irqs.
> 
> They can disable automatic in-kernel affinity.

This still means code needs to be changed ...
Anyway, what's the interface for that?

> > 
> > > (note it depends on how the APIC is
> > > configured, your patch ignores that).
> > 
> > Could you clarify please? What is meant by 'it' in 'it depends'?
> 
> "It" means the target vcpu selection. It depends on how the guest
> APIC is programmed.
> 
> > Which APIC - host or guest - do you mean, and what are possible APIC
> > configurations to consider?
> 
> Guest APIC. Guest APIC programmed with round robin would break the
> static assignment on your patch.

For round robin we might just want to disable this
automatic affinity?

> Configurations to consider, all common ones used for assigned devices?

I mean, besides round robin, any other modes that
have an issue? Interrupts can also be multicast,
I think, but we probably don't care what happens
to affinity then, as msi interrupts are probably never
broadcast ...

> 
> > > >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > >  		struct kvm_lapic_irq *irq, int host_irq)
> > > >  {
> > > > @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > >  			if (r < 0)
> > > >  				r = 0;
> > > >  			r += kvm_apic_set_irq(vcpu, irq);
> > > > +			kvm_vcpu_host_irq_hint(vcpu, host_irq);
> > > >  		} else if (kvm_lapic_enabled(vcpu)) {
> > > >  			if (!lowest)
> > > >  				lowest = vcpu;
> > > > @@ -110,8 +123,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if (lowest)
> > > > +	if (lowest) {
> > > >  		r = kvm_apic_set_irq(lowest, irq);
> > > > +		kvm_vcpu_host_irq_hint(vcpu, host_irq);
> > > > +	}
> > > >  
> > > >  	return r;
> > > >  }
> > > > -- 
> > > > 1.7.5.53.gc233e

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 09:09:46AM -0200, Marcelo Tosatti wrote:
> On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > > To forward an interrupt to a vcpu that runs on
> > > > a host cpu different from the current one,
> > > > we need an ipi which likely will cost us as much
> > > > as delivering the interrupt directly to that cpu would.
> > > > 
> > > > Set irq affinity hint to point there, irq balancer
> > > > can then take this into accound and balance
> > > > interrupts accordingly.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  virt/kvm/assigned-dev.c |    8 +++++---
> > > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > > >  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> Or maybe even automatic in-kernel irq affinity disabled by 
> default (to avoid interference with userspace pinning). 

Right. But what's the interface to enable it?
There's no standard interface, is there?

The advantage of hinting is that it works
with existing tools, without changing userspace at all.

> All of that if it makes sense performance wise, of course.

That's the point. Almost any benchmark I'm aware of is run with vcpus
pinned.

-- 
MST

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Marcelo Tosatti]

On Mon, Oct 17, 2011 at 03:32:15PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 08:58:59AM -0200, Marcelo Tosatti wrote:
> > On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > > > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > > > To forward an interrupt to a vcpu that runs on
> > > > > a host cpu different from the current one,
> > > > > we need an ipi which likely will cost us as much
> > > > > as delivering the interrupt directly to that cpu would.
> > > > > 
> > > > > Set irq affinity hint to point there, irq balancer
> > > > > can then take this into accound and balance
> > > > > interrupts accordingly.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  virt/kvm/assigned-dev.c |    8 +++++---
> > > > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > > > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > index f89f138..b579777 100644
> > > > > --- a/virt/kvm/assigned-dev.c
> > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
> > > > >  		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > > >  			disable_irq(assigned_dev->host_msix_entries[i].vector);
> > > > >  
> > > > > -		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > > > -			free_irq(assigned_dev->host_msix_entries[i].vector,
> > > > > -				 (void *)assigned_dev);
> > > > > +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> > > > > +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> > > > > +			irq_set_affinity_hint(vector, NULL);
> > > > > +			free_irq(vector, (void *)assigned_dev);
> > > > > +		}
> > > > >  
> > > > >  		assigned_dev->entries_nr = 0;
> > > > >  		kfree(assigned_dev->host_msix_entries);
> > > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > > index ac8b629..68b1f7c 100644
> > > > > --- a/virt/kvm/irq_comm.c
> > > > > +++ b/virt/kvm/irq_comm.c
> > > > > @@ -22,6 +22,7 @@
> > > > >  
> > > > >  #include <linux/kvm_host.h>
> > > > >  #include <linux/slab.h>
> > > > > +#include <linux/interrupt.h>
> > > > >  #include <trace/events/kvm.h>
> > > > >  
> > > > >  #include <asm/msidef.h>
> > > > > @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > > >  #endif
> > > > >  }
> > > > >  
> > > > > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > > > > +{
> > > > > +	const struct cpumask *mask;
> > > > > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > > > > +	 * wrong value but we don't mind much. */
> > > > > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > > > > +		mask = get_cpu_mask(vcpu->cpu);
> > > > > +		irq_set_affinity_hint(host_irq, mask);
> > > > > +	}
> > > > > +}
> > > > 
> > > > Unsure about the internals of irq_set_affinity_hint, but AFAICS its
> > > > exported so that irqbalance in userspace can make a decision.
> > > 
> > > Yes. Pls note at the moment there's no hint so irqbalance
> > > will likely try to move the irq away from vcpu if that
> > > is doing a lot of work. My patch tries to correct that.
> > > 
> > > > If that is the case, then irqbalance update rate should be high enough
> > > > to catch up with a vcpu migrating betweens cpus (which initially does
> > > > not appear a sensible arrangement).
> > > 
> > > At least for pinned vcpus, that's almost sure to be the case :)
> > 
> > What i mean is that the frequency of a vcpu migrating between cpus
> > might be higher than what irqbalance can cope with.
> > 
> > > > The decision to have the host interrupt follow the vcpu seems a good
> > > > one, given that it saves an IPI and is potentially more cache friendly
> > > > overall.
> > > 
> > > > And AFAICS its more intelligent for the device assignment case than
> > > > anything irqbalance can come up with
> > > 
> > > Do you just propose overwriting affinity set by userspace then?
> > 
> > Yes.
> > 
> > > My concern would be to avoid breaking setups some users have,
> > > with carefully manually optimized affinity for vcpus and device irqs.
> > 
> > They can disable automatic in-kernel affinity.
> 
> This still means code needs to be changed ...
> Anyway, what's the interface for that?
> 
> > > 
> > > > (note it depends on how the APIC is
> > > > configured, your patch ignores that).
> > > 
> > > Could you clarify please? What is meant by 'it' in 'it depends'?
> > 
> > "It" means the target vcpu selection. It depends on how the guest
> > APIC is programmed.
> > 
> > > Which APIC - host or guest - do you mean, and what are possible APIC
> > > configurations to consider?
> > 
> > Guest APIC. Guest APIC programmed with round robin would break the
> > static assignment on your patch.
> 
> For round robin we might just want to disable this
> automatic affinity?

OK.

> > Configurations to consider, all common ones used for assigned devices?
> 
> I mean, besides round robin, any other modes that
> have an issue? Interrupts can also be multicast,
> I think, but we probably don't care what happens
> to affinity then, as msi interrupts are probably never
> broadcast ...

There is also lowest priority, which can be used with MSI.

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 02:07:41PM -0200, Marcelo Tosatti wrote:
> On Mon, Oct 17, 2011 at 03:32:15PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 17, 2011 at 08:58:59AM -0200, Marcelo Tosatti wrote:
> > > On Sun, Oct 16, 2011 at 03:12:23PM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 13, 2011 at 11:54:50AM -0300, Marcelo Tosatti wrote:
> > > > > On Tue, Oct 11, 2011 at 08:38:28PM +0200, Michael S. Tsirkin wrote:
> > > > > > To forward an interrupt to a vcpu that runs on
> > > > > > a host cpu different from the current one,
> > > > > > we need an ipi which likely will cost us as much
> > > > > > as delivering the interrupt directly to that cpu would.
> > > > > > 
> > > > > > Set irq affinity hint to point there, irq balancer
> > > > > > can then take this into accound and balance
> > > > > > interrupts accordingly.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >  virt/kvm/assigned-dev.c |    8 +++++---
> > > > > >  virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
> > > > > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> > > > > > index f89f138..b579777 100644
> > > > > > --- a/virt/kvm/assigned-dev.c
> > > > > > +++ b/virt/kvm/assigned-dev.c
> > > > > > @@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
> > > > > >  		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > > > >  			disable_irq(assigned_dev->host_msix_entries[i].vector);
> > > > > >  
> > > > > > -		for (i = 0; i < assigned_dev->entries_nr; i++)
> > > > > > -			free_irq(assigned_dev->host_msix_entries[i].vector,
> > > > > > -				 (void *)assigned_dev);
> > > > > > +		for (i = 0; i < assigned_dev->entries_nr; i++) {
> > > > > > +			u32 vector = assigned_dev->host_msix_entries[i].vector;
> > > > > > +			irq_set_affinity_hint(vector, NULL);
> > > > > > +			free_irq(vector, (void *)assigned_dev);
> > > > > > +		}
> > > > > >  
> > > > > >  		assigned_dev->entries_nr = 0;
> > > > > >  		kfree(assigned_dev->host_msix_entries);
> > > > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > > > index ac8b629..68b1f7c 100644
> > > > > > --- a/virt/kvm/irq_comm.c
> > > > > > +++ b/virt/kvm/irq_comm.c
> > > > > > @@ -22,6 +22,7 @@
> > > > > >  
> > > > > >  #include <linux/kvm_host.h>
> > > > > >  #include <linux/slab.h>
> > > > > > +#include <linux/interrupt.h>
> > > > > >  #include <trace/events/kvm.h>
> > > > > >  
> > > > > >  #include <asm/msidef.h>
> > > > > > @@ -80,6 +81,17 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > > > >  #endif
> > > > > >  }
> > > > > >  
> > > > > > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > > > > > +{
> > > > > > +	const struct cpumask *mask;
> > > > > > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > > > > > +	 * wrong value but we don't mind much. */
> > > > > > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > > > > > +		mask = get_cpu_mask(vcpu->cpu);
> > > > > > +		irq_set_affinity_hint(host_irq, mask);
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > Unsure about the internals of irq_set_affinity_hint, but AFAICS its
> > > > > exported so that irqbalance in userspace can make a decision.
> > > > 
> > > > Yes. Pls note at the moment there's no hint so irqbalance
> > > > will likely try to move the irq away from vcpu if that
> > > > is doing a lot of work. My patch tries to correct that.
> > > > 
> > > > > If that is the case, then irqbalance update rate should be high enough
> > > > > to catch up with a vcpu migrating betweens cpus (which initially does
> > > > > not appear a sensible arrangement).
> > > > 
> > > > At least for pinned vcpus, that's almost sure to be the case :)
> > > 
> > > What i mean is that the frequency of a vcpu migrating between cpus
> > > might be higher than what irqbalance can cope with.
> > > 
> > > > > The decision to have the host interrupt follow the vcpu seems a good
> > > > > one, given that it saves an IPI and is potentially more cache friendly
> > > > > overall.
> > > > 
> > > > > And AFAICS its more intelligent for the device assignment case than
> > > > > anything irqbalance can come up with
> > > > 
> > > > Do you just propose overwriting affinity set by userspace then?
> > > 
> > > Yes.
> > > 
> > > > My concern would be to avoid breaking setups some users have,
> > > > with carefully manually optimized affinity for vcpus and device irqs.
> > > 
> > > They can disable automatic in-kernel affinity.
> > 
> > This still means code needs to be changed ...
> > Anyway, what's the interface for that?
> > 
> > > > 
> > > > > (note it depends on how the APIC is
> > > > > configured, your patch ignores that).
> > > > 
> > > > Could you clarify please? What is meant by 'it' in 'it depends'?
> > > 
> > > "It" means the target vcpu selection. It depends on how the guest
> > > APIC is programmed.
> > > 
> > > > Which APIC - host or guest - do you mean, and what are possible APIC
> > > > configurations to consider?
> > > 
> > > Guest APIC. Guest APIC programmed with round robin would break the
> > > static assignment on your patch.
> > 
> > For round robin we might just want to disable this
> > automatic affinity?
> 
> OK.
> 
> > > Configurations to consider, all common ones used for assigned devices?
> > 
> > I mean, besides round robin, any other modes that
> > have an issue? Interrupts can also be multicast,
> > I think, but we probably don't care what happens
> > to affinity then, as msi interrupts are probably never
> > broadcast ...
> 
> There is also lowest priority, which can be used with MSI.

What's the best thing to do there, in your opinion?

Also, if the *guest* sets the affinity, is there some
way to observe that on the host?

-- 
MST

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 02:07:41PM -0200, Marcelo Tosatti wrote:
> > > Configurations to consider, all common ones used for assigned devices?
> > 
> > I mean, besides round robin, any other modes that
> > have an issue? Interrupts can also be multicast,
> > I think, but we probably don't care what happens
> > to affinity then, as msi interrupts are probably never
> > broadcast ...
> 
> There is also lowest priority, which can be used with MSI.


So the following will probably address that comment?

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index f89f138..b579777 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -142,9 +142,11 @@ static void deassign_host_irq(struct kvm *kvm,
 		for (i = 0; i < assigned_dev->entries_nr; i++)
 			disable_irq(assigned_dev->host_msix_entries[i].vector);
 
-		for (i = 0; i < assigned_dev->entries_nr; i++)
-			free_irq(assigned_dev->host_msix_entries[i].vector,
-				 (void *)assigned_dev);
+		for (i = 0; i < assigned_dev->entries_nr; i++) {
+			u32 vector = assigned_dev->host_msix_entries[i].vector;
+			irq_set_affinity_hint(vector, NULL);
+			free_irq(vector, (void *)assigned_dev);
+		}
 
 		assigned_dev->entries_nr = 0;
 		kfree(assigned_dev->host_msix_entries);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index ac8b629..dabf89f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -22,6 +22,7 @@
 
 #include <linux/kvm_host.h>
 #include <linux/slab.h>
+#include <linux/interrupt.h>
 #include <trace/events/kvm.h>
 
 #include <asm/msidef.h>
@@ -80,11 +81,23 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
 #endif
 }
 
+static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
+{
+	const struct cpumask *mask;
+	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
+	 * wrong value but we don't mind much. */
+	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
+		mask = get_cpu_mask(vcpu->cpu);
+		irq_set_affinity_hint(host_irq, mask);
+	}
+}
+	
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, int host_irq)
 {
 	int i, r = -1;
-	struct kvm_vcpu *vcpu, *lowest = NULL;
+	int matches = 0;
+	struct kvm_vcpu *vcpu, *lowest = NULL, *match;
 
 	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
 			kvm_is_dm_lowest_prio(irq))
@@ -98,10 +111,13 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 					irq->dest_id, irq->dest_mode))
 			continue;
 
+		++matches;
+
 		if (!kvm_is_dm_lowest_prio(irq)) {
 			if (r < 0)
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq);
+			match = vcpu;
 		} else if (kvm_lapic_enabled(vcpu)) {
 			if (!lowest)
 				lowest = vcpu;
@@ -110,8 +126,13 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		}
 	}
 
-	if (lowest)
+	if (lowest) {
 		r = kvm_apic_set_irq(lowest, irq);
+		match = lowest;
+	}
+
+	if (matches == 1)
+		kvm_vcpu_host_irq_hint(match, host_irq);
 
 	return r;
 }

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Marcelo Tosatti]

On Mon, Oct 17, 2011 at 07:04:40PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 02:07:41PM -0200, Marcelo Tosatti wrote:
> > > > Configurations to consider, all common ones used for assigned devices?
> > > 
> > > I mean, besides round robin, any other modes that
> > > have an issue? Interrupts can also be multicast,
> > > I think, but we probably don't care what happens
> > > to affinity then, as msi interrupts are probably never
> > > broadcast ...
> > 
> > There is also lowest priority, which can be used with MSI.
> 
> 
> So the following will probably address that comment?

Yes, it does. Patch looks fine.

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Avi Kivity]

On 10/11/2011 08:38 PM, Michael S. Tsirkin wrote:
> To forward an interrupt to a vcpu that runs on
> a host cpu different from the current one,
> we need an ipi which likely will cost us as much
> as delivering the interrupt directly to that cpu would.
>
> Set irq affinity hint to point there, irq balancer
> can then take this into accound and balance
> interrupts accordingly.
>
>  
> +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> +{
> +	const struct cpumask *mask;
> +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> +	 * wrong value but we don't mind much. */
> +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> +		mask = get_cpu_mask(vcpu->cpu);
> +		irq_set_affinity_hint(host_irq, mask);
> +	}
> +}
> +	
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, int host_irq)
>  {
> @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  			if (r < 0)
>  				r = 0;
>  			r += kvm_apic_set_irq(vcpu, irq);
> +			kvm_vcpu_host_irq_hint(vcpu, host_irq);

Doing this every time seems excessive.  How about doing it every N
interrupts?  We can even collect information about which vcpus were
targeted, and then use a mask instead of just one vcpu.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH RFC 2/2] kvm: set affinity hint for assigned device msi

[Michael S. Tsirkin]

On Thu, Jan 12, 2012 at 04:09:17PM +0200, Avi Kivity wrote:
> On 10/11/2011 08:38 PM, Michael S. Tsirkin wrote:
> > To forward an interrupt to a vcpu that runs on
> > a host cpu different from the current one,
> > we need an ipi which likely will cost us as much
> > as delivering the interrupt directly to that cpu would.
> >
> > Set irq affinity hint to point there, irq balancer
> > can then take this into accound and balance
> > interrupts accordingly.
> >
> >  
> > +static void kvm_vcpu_host_irq_hint(struct kvm_vcpu *vcpu, int host_irq)
> > +{
> > +	const struct cpumask *mask;
> > +	/* raw_smp_processor_id() is ok here: if we get preempted we can get a
> > +	 * wrong value but we don't mind much. */
> > +	if (host_irq >= 0 && unlikely(vcpu->cpu != raw_smp_processor_id())) {
> > +		mask = get_cpu_mask(vcpu->cpu);
> > +		irq_set_affinity_hint(host_irq, mask);
> > +	}
> > +}
> > +	
> >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  		struct kvm_lapic_irq *irq, int host_irq)
> >  {
> > @@ -102,6 +114,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  			if (r < 0)
> >  				r = 0;
> >  			r += kvm_apic_set_irq(vcpu, irq);
> > +			kvm_vcpu_host_irq_hint(vcpu, host_irq);
> 
> Doing this every time seems excessive.

Please note this doesn't modify the mapping in the hardware:
it only updates the hint for the irqbalancer,
and only if we target a CPU which is different
from the current one.

> How about doing it every N interrupts?

That's certainly easy.

> We can even collect information about which vcpus were
> targeted, and then use a mask instead of just one vcpu.

Yes, we can, but it certainly becomes much more complex.
The motivatin behind using the same host cpu is reducing the
number of IPIs. If the interrupt is bouncing between
guest VCPUs, this optimizaion won't work, so what's the
theory behind using a mask?
How about detecting an unstable vcpu mapping and
completely disabling the optimization, instead?

> -- 
> error compiling committee.c: too many arguments to function