apic: eoi optimization support

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
Guest tests it using a single est and clear operation - this is necessary so that host can detect interrupt nesting - and if set, it can skip the EOI MSR.

This patch is not final yet, pls don't apply
until host side is also finalized. Including
it here for completeness to show another user
of the new eoi_write interface.

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

---
 arch/x86/include/asm/bitops.h   |    6 +++-
 arch/x86/include/asm/kvm_para.h |    2 +
 arch/x86/kernel/kvm.c           |   51 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 5 deletions(-)

arch/x86/include/asm/bitops.h +4 -2

index b97596e..c9c70ea 100644
@@ -26,11 +26,13 @@
 #if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
 /* Technically wrong, but this avoids compilation errors on some gcc
    versions. */
-#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "=m"
 #else
-#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "+m"
 #endif
 
+#define BITOP_ADDR(x) BITOP_ADDR_CONSTRAINT (*(volatile long *) (x))
+
 #define ADDR				BITOP_ADDR(addr)
 
 /*

arch/x86/include/asm/kvm_para.h +2 -0

index 734c376..195840d 100644
@@ -22,6 +22,7 @@
 #define KVM_FEATURE_CLOCKSOURCE2        3
 #define KVM_FEATURE_ASYNC_PF		4
 #define KVM_FEATURE_STEAL_TIME		5
+#define KVM_FEATURE_EOI			6
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.

@@ -37,6 +38,7 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
 
 struct kvm_steal_time {
 	__u64 steal;

arch/x86/kernel/kvm.c +41 -3

index b8ba6e4..ad66cdd 100644
@@ -39,6 +39,8 @@
 #include <asm/desc.h>
 #include <asm/tlbflush.h>
 #include <asm/idle.h>
+#include <asm/apic.h>
+#include <asm/apicdef.h>
 
 static int kvmapf = 1;
 

@@ -290,6 +292,27 @@ static void kvm_register_steal_time(void)
 		cpu, __pa(st));
 }
 
+static DEFINE_PER_CPU(u16, kvm_apic_eoi) __aligned(2) = 0;
+
+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */
+static inline int kvm_test_and_clear_bit(int nr, volatile u16* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit), BITOP_ADDR_CONSTRAINT(*addr) : "Ir" (nr));
+	return oldbit;
+}
+
+static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
+{
+	if (kvm_test_and_clear_bit(0, &__get_cpu_var(kvm_apic_eoi)))
+		return;
+	apic->write(APIC_EOI, APIC_EOI_ACK);
+}
+
 void __cpuinit kvm_guest_cpu_init(void)
 {
 	if (!kvm_para_available())

@@ -307,11 +330,17 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	if (kvm_para_has_feature(KVM_FEATURE_EOI)) {
+		__get_cpu_var(kvm_apic_eoi) = 0;
+		wrmsrl(MSR_KVM_EOI_EN, __pa(&__get_cpu_var(kvm_apic_eoi)) |
+		       KVM_MSR_ENABLED);
+	}
+
 	if (has_steal_clock)
 		kvm_register_steal_time();
 }
 
-static void kvm_pv_disable_apf(void *unused)
+static void kvm_pv_disable_apf(void)
 {
 	if (!__get_cpu_var(apf_reason).enabled)
 		return;

@@ -323,11 +352,18 @@ static void kvm_pv_disable_apf(void *unused)
 	       smp_processor_id());
 }
 
+static void kvm_pv_guest_cpu_reboot(void *unused)
+{
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
+}
+
 static int kvm_pv_reboot_notify(struct notifier_block *nb,
 				unsigned long code, void *unused)
 {
 	if (code == SYS_RESTART)
-		on_each_cpu(kvm_pv_disable_apf, NULL, 1);
+		on_each_cpu(kvm_pv_guest_cpu_reboot, NULL, 1);
 	return NOTIFY_DONE;
 }
 

@@ -378,7 +414,9 @@ static void __cpuinit kvm_guest_cpu_online(void *dummy)
 static void kvm_guest_cpu_offline(void *dummy)
 {
 	kvm_disable_steal_time();
-	kvm_pv_disable_apf(NULL);
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
 	apf_task_wake_all();
 }
 

arch/x86/kernel/kvm.c +4 -0

+		BUG_ON(apic->eoi_write == kvm_guest_apic_eoi_write);
+		apic->eoi_write = kvm_guest_apic_eoi_write;
+	}
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;
 	register_cpu_notifier(&kvm_cpu_notifier);

Signature · 1 line

1.7.9.111.gf3fb0

Quoted 6 lines · Michael S. Tsirkin

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
Guest tests it using a single est and clear operation - this is necessary so that host can detect interrupt nesting - and if set, it can skip the EOI MSR.

Quoted 6 lines

This patch is not final yet, pls don't apply
until host side is also finalized. Including
it here for completeness to show another user
of the new eoi_write interface.

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

Quoted 137 lines

---
 arch/x86/include/asm/bitops.h   |    6 +++-
 arch/x86/include/asm/kvm_para.h |    2 +
 arch/x86/kernel/kvm.c           |   51 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 5 deletions(-)

arch/x86/include/asm/bitops.h +4 -2

index b97596e..c9c70ea 100644
@@ -26,11 +26,13 @@
 #if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
 /* Technically wrong, but this avoids compilation errors on some gcc
    versions. */
-#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "=m"
 #else
-#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "+m"
 #endif
 
+#define BITOP_ADDR(x) BITOP_ADDR_CONSTRAINT (*(volatile long *) (x))
+
 #define ADDR				BITOP_ADDR(addr)
 
 /*

arch/x86/include/asm/kvm_para.h +2 -0

index 734c376..195840d 100644
@@ -22,6 +22,7 @@
 #define KVM_FEATURE_CLOCKSOURCE2        3
 #define KVM_FEATURE_ASYNC_PF		4
 #define KVM_FEATURE_STEAL_TIME		5
+#define KVM_FEATURE_EOI			6
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.

@@ -37,6 +38,7 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
 
 struct kvm_steal_time {
 	__u64 steal;

arch/x86/kernel/kvm.c +41 -3

index b8ba6e4..ad66cdd 100644
@@ -39,6 +39,8 @@
 #include <asm/desc.h>
 #include <asm/tlbflush.h>
 #include <asm/idle.h>
+#include <asm/apic.h>
+#include <asm/apicdef.h>
 
 static int kvmapf = 1;
 

@@ -290,6 +292,27 @@ static void kvm_register_steal_time(void)
 		cpu, __pa(st));
 }
 
+static DEFINE_PER_CPU(u16, kvm_apic_eoi) __aligned(2) = 0;
+
+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */
+static inline int kvm_test_and_clear_bit(int nr, volatile u16* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit), BITOP_ADDR_CONSTRAINT(*addr) : "Ir" (nr));
+	return oldbit;
+}
+
+static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
+{
+	if (kvm_test_and_clear_bit(0, &__get_cpu_var(kvm_apic_eoi)))
+		return;
+	apic->write(APIC_EOI, APIC_EOI_ACK);
+}
+
 void __cpuinit kvm_guest_cpu_init(void)
 {
 	if (!kvm_para_available())

@@ -307,11 +330,17 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	if (kvm_para_has_feature(KVM_FEATURE_EOI)) {
+		__get_cpu_var(kvm_apic_eoi) = 0;
+		wrmsrl(MSR_KVM_EOI_EN, __pa(&__get_cpu_var(kvm_apic_eoi)) |
+		       KVM_MSR_ENABLED);
+	}
+
 	if (has_steal_clock)
 		kvm_register_steal_time();
 }
 
-static void kvm_pv_disable_apf(void *unused)
+static void kvm_pv_disable_apf(void)
 {
 	if (!__get_cpu_var(apf_reason).enabled)
 		return;

@@ -323,11 +352,18 @@ static void kvm_pv_disable_apf(void *unused)
 	       smp_processor_id());
 }
 
+static void kvm_pv_guest_cpu_reboot(void *unused)
+{
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
+}
+
 static int kvm_pv_reboot_notify(struct notifier_block *nb,
 				unsigned long code, void *unused)
 {
 	if (code == SYS_RESTART)
-		on_each_cpu(kvm_pv_disable_apf, NULL, 1);
+		on_each_cpu(kvm_pv_guest_cpu_reboot, NULL, 1);
 	return NOTIFY_DONE;
 }
 

@@ -378,7 +414,9 @@ static void __cpuinit kvm_guest_cpu_online(void *dummy)
 static void kvm_guest_cpu_offline(void *dummy)
 {
 	kvm_disable_steal_time();
-	kvm_pv_disable_apf(NULL);
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
 	apf_task_wake_all();
 }
 

arch/x86/kernel/kvm.c void __init kvm_guest_init(void) · line 474

Quoted 7 lines

arch/x86/kernel/kvm.c +3 -0

@@ -431,6 +469,13 @@ void __init kvm_guest_init(void)
 		pv_time_ops.steal_clock = kvm_steal_clock;
 	}
 
+	if (kvm_para_has_feature(KVM_FEATURE_EOI)) {
+		/* Should happen once after apic is initialized */
+		BUG_ON(!apic);

What if kernel was started with noapic?

Quoted 9 lines

arch/x86/kernel/kvm.c +4 -0

+		BUG_ON(apic->eoi_write == kvm_guest_apic_eoi_write);
+		apic->eoi_write = kvm_guest_apic_eoi_write;
+	}
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;
 	register_cpu_notifier(&kvm_cpu_notifier);

Signature · 1 line

1.7.9.111.gf3fb0

--
Gleb.

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

The idea is simple: there's a bit, per APIC, in guest memory, that tells the guest that it does not need EOI.
Guest tests it using a single est and clear operation - this is necessary so that host can detect interrupt nesting - and if set, it can skip the EOI MSR.

Quoted 6 lines

Quoted 6 lines

This patch is not final yet, pls don't apply
until host side is also finalized. Including
it here for completeness to show another user
of the new eoi_write interface.

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

Just pointing out the above: this last patch in the series is for illustrative purposes at this point.

Quoted 141 lines

Quoted 141 lines

---
 arch/x86/include/asm/bitops.h   |    6 +++-
 arch/x86/include/asm/kvm_para.h |    2 +
 arch/x86/kernel/kvm.c           |   51 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 5 deletions(-)

arch/x86/include/asm/bitops.h +4 -2

index b97596e..c9c70ea 100644
@@ -26,11 +26,13 @@
 #if __GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 1)
 /* Technically wrong, but this avoids compilation errors on some gcc
    versions. */
-#define BITOP_ADDR(x) "=m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "=m"
 #else
-#define BITOP_ADDR(x) "+m" (*(volatile long *) (x))
+#define BITOP_ADDR_CONSTRAINT "+m"
 #endif
 
+#define BITOP_ADDR(x) BITOP_ADDR_CONSTRAINT (*(volatile long *) (x))
+
 #define ADDR				BITOP_ADDR(addr)
 
 /*

arch/x86/include/asm/kvm_para.h +2 -0

index 734c376..195840d 100644
@@ -22,6 +22,7 @@
 #define KVM_FEATURE_CLOCKSOURCE2        3
 #define KVM_FEATURE_ASYNC_PF		4
 #define KVM_FEATURE_STEAL_TIME		5
+#define KVM_FEATURE_EOI			6
 
 /* The last 8 bits are used to indicate how to interpret the flags field
  * in pvclock structure. If no bits are set, all flags are ignored.

@@ -37,6 +38,7 @@
 #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01
 #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
 #define MSR_KVM_STEAL_TIME  0x4b564d03
+#define MSR_KVM_EOI_EN      0x4b564d04
 
 struct kvm_steal_time {
 	__u64 steal;

arch/x86/kernel/kvm.c +41 -3

index b8ba6e4..ad66cdd 100644
@@ -39,6 +39,8 @@
 #include <asm/desc.h>
 #include <asm/tlbflush.h>
 #include <asm/idle.h>
+#include <asm/apic.h>
+#include <asm/apicdef.h>
 
 static int kvmapf = 1;
 

@@ -290,6 +292,27 @@ static void kvm_register_steal_time(void)
 		cpu, __pa(st));
 }
 
+static DEFINE_PER_CPU(u16, kvm_apic_eoi) __aligned(2) = 0;
+
+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */
+static inline int kvm_test_and_clear_bit(int nr, volatile u16* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit), BITOP_ADDR_CONSTRAINT(*addr) : "Ir" (nr));
+	return oldbit;
+}
+
+static void kvm_guest_apic_eoi_write(u32 reg, u32 val)
+{
+	if (kvm_test_and_clear_bit(0, &__get_cpu_var(kvm_apic_eoi)))
+		return;
+	apic->write(APIC_EOI, APIC_EOI_ACK);
+}
+
 void __cpuinit kvm_guest_cpu_init(void)
 {
 	if (!kvm_para_available())

@@ -307,11 +330,17 @@ void __cpuinit kvm_guest_cpu_init(void)
 		       smp_processor_id());
 	}
 
+	if (kvm_para_has_feature(KVM_FEATURE_EOI)) {
+		__get_cpu_var(kvm_apic_eoi) = 0;
+		wrmsrl(MSR_KVM_EOI_EN, __pa(&__get_cpu_var(kvm_apic_eoi)) |
+		       KVM_MSR_ENABLED);
+	}
+
 	if (has_steal_clock)
 		kvm_register_steal_time();
 }
 
-static void kvm_pv_disable_apf(void *unused)
+static void kvm_pv_disable_apf(void)
 {
 	if (!__get_cpu_var(apf_reason).enabled)
 		return;

@@ -323,11 +352,18 @@ static void kvm_pv_disable_apf(void *unused)
 	       smp_processor_id());
 }
 
+static void kvm_pv_guest_cpu_reboot(void *unused)
+{
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
+}
+
 static int kvm_pv_reboot_notify(struct notifier_block *nb,
 				unsigned long code, void *unused)
 {
 	if (code == SYS_RESTART)
-		on_each_cpu(kvm_pv_disable_apf, NULL, 1);
+		on_each_cpu(kvm_pv_guest_cpu_reboot, NULL, 1);
 	return NOTIFY_DONE;
 }
 

@@ -378,7 +414,9 @@ static void __cpuinit kvm_guest_cpu_online(void *dummy)
 static void kvm_guest_cpu_offline(void *dummy)
 {
 	kvm_disable_steal_time();
-	kvm_pv_disable_apf(NULL);
+	if (kvm_para_has_feature(KVM_FEATURE_EOI))
+		wrmsrl(MSR_KVM_EOI_EN, 0);
+	kvm_pv_disable_apf();
 	apf_task_wake_all();
 }
 

@@ -431,6 +469,13 @@ void __init kvm_guest_init(void)
 		pv_time_ops.steal_clock = kvm_steal_clock;
 	}
 

Quoted 4 lines

arch/x86/kernel/kvm.c void __init kvm_guest_init(void) · line 474

Quoted 3 lines

arch/x86/kernel/kvm.c +3 -0

+	if (kvm_para_has_feature(KVM_FEATURE_EOI)) {
+		/* Should happen once after apic is initialized */
+		BUG_ON(!apic);

What if kernel was started with noapic?

I didn't test this yet. I thought noapic sets the apic pointer to apic_noop, not to NULL.

So this case will get slowed down slightly if it actually runs on kvm. It does not look like noapic guest is worth optimising for.

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/kvm.c +4 -0

+		BUG_ON(apic->eoi_write == kvm_guest_apic_eoi_write);
+		apic->eoi_write = kvm_guest_apic_eoi_write;
+	}
+

Quoted 8 lines

Quoted 5 lines

arch/x86/kernel/kvm.c +0 -0

 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;
 	register_cpu_notifier(&kvm_cpu_notifier);

Signature · 1 line

1.7.9.111.gf3fb0

--
Gleb.

+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */

arch/x86/kernel/kvm.c +8 -0

+static inline int kvm_test_and_clear_bit(int nr, volatile u16* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit), BITOP_ADDR_CONSTRAINT(*addr) : "Ir" (nr));
+	return oldbit;

Il 23/04/2012 16:04, Michael S. Tsirkin ha scritto:

Quoted 2 lines

+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */

Is this for microoptimization or correctness? If the latter, it does not ensure anything without a "lock" prefix.

Paolo

arch/x86/kernel/kvm.c static void kvm_register_steal_time(void) · line 306

Quoted 8 lines

arch/x86/kernel/kvm.c +8 -0

+static inline int kvm_test_and_clear_bit(int nr, volatile u16* addr)
+{
+	int oldbit;
+
+	asm volatile("btr %2,%1\n\t"
+		     "sbb %0,%0"
+		     : "=r" (oldbit), BITOP_ADDR_CONSTRAINT(*addr) : "Ir" (nr));
+	return oldbit;

Quoted 7 lines · Paolo Bonzini

Il 23/04/2012 16:04, Michael S. Tsirkin ha scritto:

Quoted 2 lines

+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */

Is this for microoptimization or correctness? If the latter, it does not ensure anything without a "lock" prefix.

It can't race with other vcpus, only with vmexit on the same vcpu.

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Paolo Bonzini

Il 23/04/2012 16:04, Michael S. Tsirkin ha scritto:

Quoted 2 lines

+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */

Quoted 5 lines

Quoted 3 lines

Is this for microoptimization or correctness? If the latter, it does not ensure anything without a "lock" prefix.

It can't race with other vcpus, only with vmexit on the same vcpu.

That doesn't answer the question very well... I really don't understand the point of having a private copy here.

I really, really don't want a bunch of private interfaces around. It would be a lot better to define a test_and_{set,clear}_bit_local() in <asm/bitops.h> which is defined to be local CPU atomic.

Quoted 10 lines · Gleb Natapov

Quoted 7 lines · Paolo Bonzini

Il 23/04/2012 16:04, Michael S. Tsirkin ha scritto:

Quoted 2 lines

+/* Our own copy of __test_and_clear_bit to make sure
+ * it is done with a single instruction */

Is this for microoptimization or correctness? If the latter, it does not ensure anything without a "lock" prefix.

It can't race with other vcpus, only with vmexit on the same vcpu.

That doesn't answer the question very well... I really don't understand the point of having a private copy here.

I really, really don't want a bunch of private interfaces around. It would be a lot better to define a test_and_{set,clear}_bit_local() in <asm/bitops.h> which is defined to be local CPU atomic.

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Will do. OK to only define them for x86 for now?
They would be unused on other arches even if I defined them ...

Quoted 4 lines · Michael S. Tsirkin

Will do. OK to only define them for x86 for now?
They would be unused on other arches even if I defined them ...

I don't know if it's possible/practical to define them so they fall back to the fully atomic versions unless arch-overridden, but that would be the best, I would think.

I'm looking at reducing the interrupt overhead for virtualized guests:
some workloads spend a large part of their time processing interrupts. This patchset supplies infrastructure to reduce the IRQ ack overhead on
x86: the idea is to add an eoi_write callback that we can then optimize without touching other apic functionality.

The main user will be kvm: on kvm, an EOI write from the guest causes an expensive exit to host; we can avoid this using shared memory as the last patch in the series demonstrates.

But I also wrote a micro-optimized version for the regular x2apic: this shaves off a branch and about 9 instructions from EOI when x2apic is used, and a comment in ack_APIC_irq implies that someone counted instructions there, at some point.

Also included in the patchset are a couple of trivial macro fixes.

The patches work fine on my boxes and I did look at the objdump output to verify that the generated code for the micro-optimization patch looks right and actually is shorter.

Some benchmark results below (not sure what kind of testing is the most appropriate) show a tiny but measureable improvement. The tests were run on an AMD box with 24 cpus.

- A clean kernel build after reboot shows
a tiny but measureable improvement in system time which means lower CPU overhead (though not measureable in total time - that is dominated by user time and fluctuates too much):

linux# reboot -f
...
linux# make clean
linux# time make -j 64 LOCALVERSION= 2>&1 > /dev/null

Before:

real    2m52.244s
user    35m53.833s
sys     6m7.194s

After:

real    2m52.827s
user    35m48.916s
sys     6m2.305s

- perf micro-benchmarks seem to consistently show a tiny improvement in total time as well but it's below the confidence level of 3 std deviations:

# ./tools/perf/perf   stat --sync --repeat 100 --null perf bench sched messaging
...
       0.414666797 seconds time elapsed ( +-  1.29% )

Performance counter stats for 'perf bench sched messaging' (100 runs):

       0.395370891 seconds time elapsed

( +- 1.04% )

# ./tools/perf/perf   stat --sync --repeat 100 --null perf bench sched pipe -l 10000
       0.307019664 seconds time elapsed
( +-  0.10% )

       0.304738024 seconds time elapsed

( +- 0.08% )

The patches are against 3.4-rc3 - let me know if
I need to rebase.

I think patches 1-2 are definitely a good idea, and patches 3-4 might be a good idea.
Please review, and consider patches 1-4 for linux 3.5.

Thanks,
MST

Michael S. Tsirkin (5):
apic: fix typo EIO_ACK -> EOI_ACK and document
apic: use symbolic APIC_EOI_ACK
x86: add apic->eoi_write callback
x86: eoi micro-optimization
kvm_para: guest side for eoi avoidance

 arch/x86/include/asm/apic.h            |   22 ++++++++++++--
 arch/x86/include/asm/apicdef.h         |    2 +-
 arch/x86/include/asm/bitops.h          |    6 ++-
 arch/x86/include/asm/kvm_para.h        |    2 +
 arch/x86/kernel/apic/apic_flat_64.c    |    2 +
 arch/x86/kernel/apic/apic_noop.c       |    1 +
 arch/x86/kernel/apic/apic_numachip.c   |    1 +
 arch/x86/kernel/apic/bigsmp_32.c       |    1 +
 arch/x86/kernel/apic/es7000_32.c       |    2 +
 arch/x86/kernel/apic/numaq_32.c        |    1 +

 arch/x86/kernel/apic/probe_32.c        |    1 +
 arch/x86/kernel/apic/summit_32.c       |    1 +
 arch/x86/kernel/apic/x2apic_cluster.c  |    1 +
 arch/x86/kernel/apic/x2apic_phys.c     |    1 +
 arch/x86/kernel/apic/x2apic_uv_x.c     |    1 +
 arch/x86/kernel/kvm.c                  |   51 ++++++++++++++++++++++++++++++--
 arch/x86/platform/visws/visws_quirks.c |    2 +-
 17 files changed, 88 insertions(+), 10 deletions(-)

Quoted 17 lines

Michael S. Tsirkin (5):
apic: fix typo EIO_ACK -> EOI_ACK and document
apic: use symbolic APIC_EOI_ACK
x86: add apic->eoi_write callback
x86: eoi micro-optimization
kvm_para: guest side for eoi avoidance

 arch/x86/include/asm/apic.h            |   22 ++++++++++++--
 arch/x86/include/asm/apicdef.h         |    2 +-
 arch/x86/include/asm/bitops.h          |    6 ++-
 arch/x86/include/asm/kvm_para.h        |    2 +
 arch/x86/kernel/apic/apic_flat_64.c    |    2 +
 arch/x86/kernel/apic/apic_noop.c       |    1 +
 arch/x86/kernel/apic/apic_numachip.c   |    1 +
 arch/x86/kernel/apic/bigsmp_32.c       |    1 +
 arch/x86/kernel/apic/es7000_32.c       |    2 +
 arch/x86/kernel/apic/numaq_32.c        |    1 +

Quoted 8 lines

 arch/x86/kernel/apic/probe_32.c        |    1 +
 arch/x86/kernel/apic/summit_32.c       |    1 +
 arch/x86/kernel/apic/x2apic_cluster.c  |    1 +
 arch/x86/kernel/apic/x2apic_phys.c     |    1 +
 arch/x86/kernel/apic/x2apic_uv_x.c     |    1 +
 arch/x86/kernel/kvm.c                  |   51 ++++++++++++++++++++++++++++++--
 arch/x86/platform/visws/visws_quirks.c |    2 +-
 17 files changed, 88 insertions(+), 10 deletions(-)

No objections from the x86 side.

In terms of advantages, could you please create perf stat runs that counts the number of MMIOs or so? That should show a pretty obvious improvement - and that is enough as proof, no need to try to reproduce the performance win in a noisy benchmark.

Quoted 26 lines · Ingo Molnar

Quoted 25 lines

Michael S. Tsirkin (5):
apic: fix typo EIO_ACK -> EOI_ACK and document
apic: use symbolic APIC_EOI_ACK
x86: add apic->eoi_write callback
x86: eoi micro-optimization
kvm_para: guest side for eoi avoidance

 arch/x86/include/asm/apic.h            |   22 ++++++++++++--
 arch/x86/include/asm/apicdef.h         |    2 +-
 arch/x86/include/asm/bitops.h          |    6 ++-
 arch/x86/include/asm/kvm_para.h        |    2 +
 arch/x86/kernel/apic/apic_flat_64.c    |    2 +
 arch/x86/kernel/apic/apic_noop.c       |    1 +
 arch/x86/kernel/apic/apic_numachip.c   |    1 +
 arch/x86/kernel/apic/bigsmp_32.c       |    1 +
 arch/x86/kernel/apic/es7000_32.c       |    2 +
 arch/x86/kernel/apic/numaq_32.c        |    1 +
 arch/x86/kernel/apic/probe_32.c        |    1 +
 arch/x86/kernel/apic/summit_32.c       |    1 +
 arch/x86/kernel/apic/x2apic_cluster.c  |    1 +
 arch/x86/kernel/apic/x2apic_phys.c     |    1 +
 arch/x86/kernel/apic/x2apic_uv_x.c     |    1 +
 arch/x86/kernel/kvm.c                  |   51 ++++++++++++++++++++++++++++++--
 arch/x86/platform/visws/visws_quirks.c |    2 +-
 17 files changed, 88 insertions(+), 10 deletions(-)

Quoted 1 line

No objections from the x86 side.

Is kvm.git a good tree to merge this through?

Quoted 5 lines

In terms of advantages, could you please create perf stat runs that counts the number of MMIOs or so? That should show a pretty obvious improvement - and that is enough as proof, no need to try to reproduce the performance win in a noisy benchmark.

You mean with kvm PV, right? On real hardware the micro-optimization removes branches and maybe cache-misses but I don't see why would it reduce MMIOs.

Quoted 4 lines

Quoted 2 lines

No objections from the x86 side.

Is kvm.git a good tree to merge this through?

Fine to me, but I haven't checked how widely it conflicts with existing bits: by the looks of it most of the linecount is on the core x86 side, while the kvm change is well concentrated.

Quoted 5 lines · Ingo Molnar

Quoted 4 lines

Quoted 2 lines

No objections from the x86 side.

Is kvm.git a good tree to merge this through?

Quoted 3 lines

Fine to me, but I haven't checked how widely it conflicts with existing bits: by the looks of it most of the linecount is on the core x86 side, while the kvm change is well concentrated.

I don't see a problem with merging though tip.git - we're close to the next merge window, and the guest side rarely causes conflicts. But please don't apply the last patch yet, I want to review it more closely (esp. with the host side).