kvm: set irq affinity for assigned devices

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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

virt/kvm/irq_comm.c +5 -1

+	
 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;
 }

Signature · 1 line

1.7.5.53.gc233e

Quoted 50 lines · 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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

virt/kvm/irq_comm.c inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) · line 93

Quoted 10 lines

virt/kvm/irq_comm.c +10 -0

+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).

Quoted 25 lines

virt/kvm/irq_comm.c +4 -1

 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;
 }

Signature · 1 line

1.7.5.53.gc233e

Quoted 51 lines · Marcelo Tosatti

Quoted 50 lines · 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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

Quoted 13 lines

virt/kvm/irq_comm.c inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) · line 93

Quoted 10 lines

virt/kvm/irq_comm.c +10 -0

+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.

Quoted 3 lines

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 :)

Quoted 6 lines

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.

Quoted 3 lines

(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?

Quoted 25 lines

Quoted 25 lines

virt/kvm/irq_comm.c +4 -1

 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;
 }

Signature · 1 line

1.7.5.53.gc233e

Quoted 70 lines · Michael S. Tsirkin

Quoted 51 lines · Marcelo Tosatti

Quoted 50 lines · 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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

Quoted 13 lines

virt/kvm/irq_comm.c inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) · line 93

Quoted 10 lines

virt/kvm/irq_comm.c +10 -0

+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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 6 lines

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

Quoted 1 line

Do you just propose overwriting affinity set by userspace then?

Yes.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

(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.

Quoted 2 lines

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?

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

virt/kvm/irq_comm.c +4 -1

 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;
 }

Signature · 1 line

1.7.5.53.gc233e

Quoted 87 lines · Marcelo Tosatti

Quoted 70 lines · Michael S. Tsirkin

Quoted 51 lines · Marcelo Tosatti

Quoted 50 lines · 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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

Quoted 13 lines

virt/kvm/irq_comm.c inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) · line 93

Quoted 10 lines

virt/kvm/irq_comm.c +10 -0

+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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 1 line

Do you just propose overwriting affinity set by userspace then?

Yes.

Quoted 2 lines

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?

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

(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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 1 line

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 ...

Quoted 109 lines · Michael S. Tsirkin

Quoted 87 lines · Marcelo Tosatti

Quoted 70 lines · Michael S. Tsirkin

Quoted 51 lines · Marcelo Tosatti

Quoted 50 lines · 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 <[email protected]>

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

virt/kvm/assigned-dev.c +5 -3

index f89f138..b579777 100644
@@ -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);

virt/kvm/irq_comm.c +1 -0

index ac8b629..68b1f7c 100644
@@ -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
 }
 

Quoted 13 lines

virt/kvm/irq_comm.c inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq) · line 93

Quoted 10 lines

virt/kvm/irq_comm.c +10 -0

+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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 1 line

Do you just propose overwriting affinity set by userspace then?

Yes.

Quoted 2 lines

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?

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

(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.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 1 line

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.

Quoted 8 lines · Marcelo Tosatti

Quoted 7 lines

Quoted 1 line

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 ...

Quoted 1 line

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

So the following will probably address that comment?

Quoted 10 lines · Marcelo Tosatti

Quoted 9 lines · 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.

Quoted 5 lines

Quoted 5 lines

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

---
 virt/kvm/assigned-dev.c |    8 +++++---
 virt/kvm/irq_comm.c     |   17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Marcelo Tosatti

Quoted 9 lines · 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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

---
 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.

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.

virt/kvm/irq_comm.c +11 -0

+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)
 {

Quoted 23 lines · 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.

virt/kvm/irq_comm.c +11 -0

+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)

virt/kvm/irq_comm.c int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src, · line 117

Quoted 6 lines

{

virt/kvm/irq_comm.c +1 -0

@@ -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.

Signature · 1 line

error compiling committee.c: too many arguments to function