Skip to contentSkip to conversation

Add support for Pause Filtering to AMD SVM

PATCH In review Mark Langsdorf 9 participants 44 messages May 5, 2009 – Aug 5, 2009

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Add support for Pause Filtering to AMD SVM

commit 6f15c833f56267baf5abdd0fbc90a81489573053

Author: Mark Langsdorf <[email protected]>
Date:   Mon May 4 15:02:38 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and ntercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +18 -0
index ef43a18..14dab13 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,12 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2094,15 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2139,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
@@ -2227,6 +2244,7 @@ static void pre_svm_run(struct vcpu_svm *svm)
 	if (svm->vcpu.cpu != cpu ||
 	    svm->asid_generation != svm_data->asid_generation)
 		new_asid(svm, svm_data);
+
 }
 
 static void svm_drop_interrupt_shadow(struct kvm_vcpu *vcpu)
virt/kvm/kvm_main.c +2 -0
index 2b73e19..e2b730d 100644
@@ -710,6 +710,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	kvm_arch_vcpu_load(vcpu, cpu);
 	put_cpu();
 }
+EXPORT_SYMBOL_GPL(vcpu_load);
 
 void vcpu_put(struct kvm_vcpu *vcpu)
 {
@@ -719,6 +720,7 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 	preempt_enable();
 	mutex_unlock(&vcpu->mutex);
 }
+EXPORT_SYMBOL_GPL(vcpu_put);
 
 static void ack_flush(void *_completed)
 {
#1 lore raw
Quoted 42 lines · Mark Langsdorf

commit 6f15c833f56267baf5abdd0fbc90a81489573053
Author: Mark Langsdorf <[email protected]>
Date:   Mon May 4 15:02:38 2009 -0500

   New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count.  If Pause Filter Count is greater than 0 and ntercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting.  When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

   This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

   Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand.  Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

   Processor support for this feature is indicated by a CPUID bit.

   On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%.  Further performance improvement may be possible with a more sophisticated yield algorithm.

   -Mark Langsdorf
   Operating System Research Center
   AMD

arch/x86/kvm/svm.c +0 -0
index ef43a18..14dab13 100644

arch/x86/kvm/svm.c static void init_vmcb(struct vcpu_svm *svm) · line 581

Quoted 6 lines
arch/x86/kvm/svm.c +3 -0
@@ -575,6 +576,12 @@ static void init_vmcb(struct vcpu_svm *svm)
 
        svm->nested_vmcb = 0;
        svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+       if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+               control->pause_filter_count = 5000;

The commit message says something about 3000.

Quoted 6 lines
arch/x86/kvm/svm.c +3 -0
+               control->intercept |= (1ULL << INTERCEPT_PAUSE);
+       }
+
  }
 
  static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
#2 lore raw
In reply to Mark Langsdorf

Beside the small style problem the patch looks good. Please remove this empty line from the patch and resubmit. Please also add [email protected] and [email protected] to the CC list.

Joerg

Quoted 101 lines · Mark Langsdorf

commit 6f15c833f56267baf5abdd0fbc90a81489573053

Author: Mark Langsdorf <[email protected]>
Date:   Mon May 4 15:02:38 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and ntercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +17 -0
index ef43a18..14dab13 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,12 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2094,15 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2139,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,

arch/x86/kvm/svm.c static void pre_svm_run(struct vcpu_svm *svm) · line 2248

Quoted 5 lines
arch/x86/kvm/svm.c +1 -0
@@ -2227,6 +2244,7 @@ static void pre_svm_run(struct vcpu_svm *svm)
 	if (svm->vcpu.cpu != cpu ||
 	    svm->asid_generation != svm_data->asid_generation)
 		new_asid(svm, svm_data);
+
 }

Minor style nit, please move this empty line from the patch.

Quoted 22 lines
arch/x86/kvm/svm.c +0 -0
 static void svm_drop_interrupt_shadow(struct kvm_vcpu *vcpu)
virt/kvm/kvm_main.c +2 -0
index 2b73e19..e2b730d 100644
@@ -710,6 +710,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	kvm_arch_vcpu_load(vcpu, cpu);
 	put_cpu();
 }
+EXPORT_SYMBOL_GPL(vcpu_load);
 
 void vcpu_put(struct kvm_vcpu *vcpu)
 {
@@ -719,6 +720,7 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 	preempt_enable();
 	mutex_unlock(&vcpu->mutex);
 }
+EXPORT_SYMBOL_GPL(vcpu_put);
 
 static void ack_flush(void *_completed)
 {
Signature · 6 lines
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632
#3 lore raw

PATCH 1/1 Add support for Pause Filtering to AMD SVM

commit 01813db8627e74018c8cec90df7e345839351f23

Author: root <[email protected]>
Date:   Thu May 7 09:44:10 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +17 -0
index ef43a18..4279141 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,12 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2094,15 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2139,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
virt/kvm/kvm_main.c +2 -0
index 2b73e19..e2b730d 100644
@@ -710,6 +710,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	kvm_arch_vcpu_load(vcpu, cpu);
 	put_cpu();
 }
+EXPORT_SYMBOL_GPL(vcpu_load);
 
 void vcpu_put(struct kvm_vcpu *vcpu)
 {
@@ -719,6 +720,7 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 	preempt_enable();
 	mutex_unlock(&vcpu->mutex);
 }
+EXPORT_SYMBOL_GPL(vcpu_put);
 
 static void ack_flush(void *_completed)
 {
#4 lore raw

(copying Ingo)

Quoted 31 lines · Mark Langsdorf

commit 01813db8627e74018c8cec90df7e345839351f23

Author: root <[email protected]>
Date:   Thu May 7 09:44:10 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Quoted 6 lines

-Mark Langsdorf
Operating System Research Center
AMD

(please use git format-patch rather than git show, and set up user.name and user.email properly)

arch/x86/kvm/svm.c static void init_vmcb(struct vcpu_svm *svm) · line 585

Quoted 10 lines
arch/x86/kvm/svm.c +6 -0
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+
 }

3000 or 5000?

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm, · line 2105

Quoted 9 lines
arch/x86/kvm/svm.c +8 -0
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+

Ingo, will this do anything under CFS, or will CFS note that nothing has changed in the accounting and reschedule us immediately?

Signature · 1 line
error compiling committee.c: too many arguments to function
#5 lore raw
Quoted 5 lines · Avi Kivity
Quoted 5 lines

+static int pause_interception(struct vcpu_svm *svm, struct kvm_run
*kvm_run)

arch/x86/kvm/svm.c +3 -0
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
Quoted 8 lines

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm, · line 2105

Quoted 4 lines
arch/x86/kvm/svm.c +4 -0
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+

Ingo, will this do anything under CFS, or will CFS note that nothing has changed in the accounting and reschedule us immediately?

The scheduler will yield to another task only if the current task has become ineligible. I.e schedule() is largely a NOP on TASK_RUNNING tasks (i.e. here).

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

Ingo

#6 lore raw
Quoted 14 lines · Ingo Molnar
Quoted 13 lines

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm, · line 2105

Quoted 11 lines

+static int pause_interception(struct vcpu_svm *svm, struct kvm_run
*kvm_run)

arch/x86/kvm/svm.c +7 -0
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+

Ingo, will this do anything under CFS, or will CFS note that nothing has changed in the accounting and reschedule us immediately?

Quoted 2 lines

The scheduler will yield to another task only if the current task has become ineligible. I.e schedule() is largely a NOP on TASK_RUNNING tasks (i.e. here).

Especially on preemptible kernels, where the schedule() would have already happened if it could cause anything, IIUC.

Quoted 2 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. Other tasks can become eligible, hopefully the task holding the spinlock, and by the time we're scheduled back the long running task will have finished and released the lock.

For newer Linux as a guest we're better off paravirtualizing this, so we can tell the host which vcpu holds the lock; in this case kvm will want to say, take a couple milliseconds off my account and transfer it to this task (so called directed yield). However there's no reason to paravirtualize all cpu_relax() calls.

Signature · 1 line
error compiling committee.c: too many arguments to function
#7 lore raw
Quoted 11 lines · Avi Kivity
Quoted 3 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

Quoted 4 lines

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. [...]

Ok, with such a waiver, who could refuse?

This really needs a new kernel-internal scheduler API though, which does a lot of fancy things to do:

        se->vruntime += 1000000;

i.e. add 1 msec worth of nanoseconds to the task's timeline. (first remove it from the rbtree, then add it back, and nice-weight it as well) And only do it if there's other tasks running on this CPU or so.

_That_ would be pretty efficient, and would do the right thing when two (or more) vcpus run on the same CPU, and it would also do the right thing if there are repeated VM-exits due to pause filtering.

Please dont even think about using yield for this though - that will just add a huge hit to this task and wont result in any sane behavior - and yield is bound to some historic user-space behavior as well.

A gradual and linear back-off from the current timeline is more of a fair negotiation process between vcpus and results in more or less sane (and fair) scheduling, and no unnecessary looping.

You could even do an exponential backoff up to a limit of 1-10 msecs or so, starting at 100 usecs.

Ingo

#8 lore raw
Quoted 10 lines · Ingo Molnar
Quoted 6 lines · Avi Kivity
Quoted 2 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. [...]

Quoted 7 lines

Ok, with such a waiver, who could refuse?

This really needs a new kernel-internal scheduler API though, which does a lot of fancy things to do:

        se->vruntime += 1000000;

i.e. add 1 msec worth of nanoseconds to the task's timeline. (first remove it from the rbtree, then add it back, and nice-weight it as well)

I suspected it would be as simple as this.

Quoted 2 lines

And only do it if there's other tasks running on this CPU or so.

What would happen if there weren't? I'd guess the task would continue running (but with a warped vruntime)?

Quoted 8 lines

_That_ would be pretty efficient, and would do the right thing when two (or more) vcpus run on the same CPU, and it would also do the right thing if there are repeated VM-exits due to pause filtering.

Please dont even think about using yield for this though - that will just add a huge hit to this task and wont result in any sane behavior - and yield is bound to some historic user-space behavior as well.

A gradual and linear back-off from the current timeline is more of a fair negotiation process between vcpus and results in more or less sane (and fair) scheduling, and no unnecessary looping.

You could even do an exponential backoff up to a limit of 1-10 msecs or so, starting at 100 usecs.

Good idea, it eliminates another variable to be tuned.

Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw
Quoted 4 lines · Avi Kivity
Quoted 1 line

And only do it if there's other tasks running on this CPU or so.

What would happen if there weren't? I'd guess the task would continue running (but with a warped vruntime)?

We dont want that warping to occur - we just want to go back and burn CPU time in VM context. The problem is (as Peter pointed it out) that this hw facility is incomplete and does not give us any event (interrupt) and does not give us any event key (address we are waiting for) either.

So the next best thing to do is to go back to the guest, because that is where we make the most progress, more likely, and that is where we want to be to make progress immediately, with the shortest latency.

( Perhaps we could also increase vruntime beyond the standard
latency value to make sure any freshly woken task gets executed first if we are still looping. )

Quoted 12 lines
Quoted 12 lines

_That_ would be pretty efficient, and would do the right thing when two (or more) vcpus run on the same CPU, and it would also do the right thing if there are repeated VM-exits due to pause filtering.

Please dont even think about using yield for this though - that will just add a huge hit to this task and wont result in any sane behavior - and yield is bound to some historic user-space behavior as well.

A gradual and linear back-off from the current timeline is more of a fair negotiation process between vcpus and results in more or less sane (and fair) scheduling, and no unnecessary looping.

Quoted 5 lines
Quoted 3 lines

You could even do an exponential backoff up to a limit of 1-10 msecs or so, starting at 100 usecs.

Good idea, it eliminates another variable to be tuned.

It could be made fully self-tuning, if the filter threshold can be tuned fast enough. (an MSR write? A VM context field update?)

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

Ingo

#10 lore raw
Quoted 12 lines · Ingo Molnar
Quoted 4 lines · Avi Kivity
Quoted 2 lines

And only do it if there's other tasks running on this CPU or so.

What would happen if there weren't? I'd guess the task would continue running (but with a warped vruntime)?

We dont want that warping to occur - we just want to go back and burn CPU time in VM context. The problem is (as Peter pointed it out) that this hw facility is incomplete and does not give us any event (interrupt) and does not give us any event key (address we are waiting for) either.

So the next best thing to do is to go back to the guest, because that is where we make the most progress, more likely, and that is where we want to be to make progress immediately, with the shortest latency.

Right, but I thought vruntime += blah would go back into the guest if there were no other runnable tasks.

Oh I see -- we'd exit immediately and warp vruntime very fast as long as we're spinning.

Quoted 4 lines

( Perhaps we could also increase vruntime beyond the standard
latency value to make sure any freshly woken task gets executed first if we are still looping. )

We could be halfway through our remaining time. We could set it to the next task + epsilon.

But that may be too aggressive. If the lock holder is on another cpu, it may well complete before the standard latency value expires. Since we're giving up cpu time potentially to other guests, we don't want to give too much.

If we wake up too soon, we'll spin for a few microseconds and yield again. So I think your 100 us + exponential backoff is best.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

_That_ would be pretty efficient, and would do the right thing when two (or more) vcpus run on the same CPU, and it would also do the right thing if there are repeated VM-exits due to pause filtering.

Please dont even think about using yield for this though - that will just add a huge hit to this task and wont result in any sane behavior - and yield is bound to some historic user-space behavior as well.

A gradual and linear back-off from the current timeline is more of a fair negotiation process between vcpus and results in more or less sane (and fair) scheduling, and no unnecessary looping.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

You could even do an exponential backoff up to a limit of 1-10 msecs or so, starting at 100 usecs.

Good idea, it eliminates another variable to be tuned.

It could be made fully self-tuning, if the filter threshold can be tuned fast enough. (an MSR write? A VM context field update?)

The latter.

Quoted 2 lines

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

Yeah, though that has a much smaller effect as it's only responsible for a few microseconds of spinning.

Signature · 1 line
error compiling committee.c: too many arguments to function
#11 lore raw
Quoted 5 lines · Avi Kivity
Quoted 2 lines

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

Yeah, though that has a much smaller effect as it's only responsible for a few microseconds of spinning.

3000 cycles would be 1-2 usecs. Isnt the VM exit+entry cost still in that range?

Ingo

#12 lore raw
Quoted 10 lines · Ingo Molnar
Quoted 4 lines · Avi Kivity
Quoted 2 lines

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

Yeah, though that has a much smaller effect as it's only responsible for a few microseconds of spinning.

3000 cycles would be 1-2 usecs. Isnt the VM exit+entry cost still in that range?

It's 3000 executions of rep nop, so you need to account for the entire spinlock loop body.

The Linux spinlock is

            "1:\t"
            "cmpl %0, %2\n\t"
            "je 2f\n\t"
            "rep ; nop\n\t"
            "movzwl %1, %2\n\t"
            /* don't need lfence here, because loads are in-order */
            "jmp 1b\n"

5 instructions, maybe 2-3 cycles, not counting any special rep nop overhead. Mark, any idea what the spin time is?

VM entry/exit is around 1us on the newer processors.

Signature · 1 line
error compiling committee.c: too many arguments to function
#13 lore raw
Quoted 8 lines
Quoted 6 lines · Ingo Molnar
Quoted 3 lines · Avi Kivity
Quoted 3 lines

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Yeah, though that has a much smaller effect as it's only responsible for a few microseconds of spinning.

3000 cycles would be 1-2 usecs. Isnt the VM exit+entry cost still in that range?

For the processors that support this feature, VM exit+entry is a little over 2000 cycles.

Quoted 15 lines

It's 3000 executions of rep nop, so you need to account for the entire
spinlock loop body.

The Linux spinlock is

             "1:\t"
             "cmpl %0, %2\n\t"
             "je 2f\n\t"
             "rep ; nop\n\t"
             "movzwl %1, %2\n\t"
             /* don't need lfence here, because loads are in-order */
             "jmp 1b\n"
Quoted 2 lines

5 instructions, maybe 2-3 cycles, not counting any special rep nop overhead. Mark, any idea what the spin time is?

If I'm understanding the question right, the contested spin locks are being held for 5K to 10K iterations of PAUSE. So 10K to 30K cycles if your estimate of the spinlock cycle time is correct.

-Mark Langsdorf
Operating System Research Center
AMD

#14 lore raw
Quoted 10 lines · Mark
Quoted 10 lines

The Linux spinlock is

             "1:\t"
             "cmpl %0, %2\n\t"
             "je 2f\n\t"
             "rep ; nop\n\t"
             "movzwl %1, %2\n\t"
             /* don't need lfence here, because loads are in-order */
             "jmp 1b\n"
Quoted 8 lines
Quoted 2 lines

5 instructions, maybe 2-3 cycles, not counting any special rep nop overhead. Mark, any idea what the spin time is?

If I'm understanding the question right, the contested spin locks are being held for 5K to 10K iterations of PAUSE. So 10K to 30K cycles if your estimate of the spinlock cycle time is correct.

My estimate is not very reliable. Can you measure this?

btw, I'd expect you'd get much more significant improvement on Windows. Booting 16-vcpu Windows 2008 x64 took forever on my dual core host, and my guess is spinlock contention. Of course you'd need a working yield, as Ingo said schedule() does very little.

Signature · 1 line
error compiling committee.c: too many arguments to function
#15 lore raw

RE: [PATCH][KVM][retry 1] Add support for Pause Filtering to AMDSVM

In reply to Ingo Molnar
Quoted 1 line
Quoted 1 line
Quoted 1 line

Please dont even think about using yield for this though -

Oops. I'm glad I waited to get some benchmark results before submitting that version.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

A gradual and linear back-off from the current timeline is more of a fair negotiation process between vcpus and results in more or less
sane (and fair) scheduling, and no unnecessary looping.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

You could even do an exponential backoff up to a limit of 1-10 msecs or so, starting at 100 usecs.

Good idea, it eliminates another variable to be tuned.

It could be made fully self-tuning, if the filter threshold can be tuned fast enough. (an MSR write? A VM context field update?)

VMCB field update.

So the suggestion is to add a function similar to set_task_cpu() that increases the vmruntime with an exponential backoff? Is that sufficient to cause a new VCPU to step in? I'm obviously not very familiar with the scheduler code.

Quoted 2 lines

I.e. the 3000 cycles value itself could be eliminated as well. (with just a common-sense max of say 100,000 cycles enforced)

I don't understand what you're saying here. There needs to be some value in the pause filter counter to trigger the intercept.

-Mark Langsdorf
Operating System Research Center
AMD

#16 lore raw
In reply to Avi Kivity
Quoted 26 lines · Avi Kivity
Quoted 25 lines · Ingo Molnar
Quoted 11 lines · Avi Kivity
Quoted 4 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

Quoted 5 lines

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. [...]

Ok, with such a waiver, who could refuse?

This really needs a new kernel-internal scheduler API though, which does a lot of fancy things to do:

Quoted 7 lines
Quoted 5 lines
        se->vruntime += 1000000;

i.e. add 1 msec worth of nanoseconds to the task's timeline. (first remove it from the rbtree, then add it back, and nice-weight it as well)

I suspected it would be as simple as this.

Is that thread guaranteed to run as SCHED_OTHER?

#17 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 8 lines
Quoted 6 lines

This really needs a new kernel-internal scheduler API though, which does a lot of fancy things to do:

        se->vruntime += 1000000;

i.e. add 1 msec worth of nanoseconds to the task's timeline. (first remove it from the rbtree, then add it back, and nice-weight it as well)

I suspected it would be as simple as this.

Is that thread guaranteed to run as SCHED_OTHER?

No, it's user specified. But if it isn't SCHED_OTHER, we don't mind if it spins (and we hope realtime systems aren't overcommitted).

Signature · 1 line
error compiling committee.c: too many arguments to function
#18 lore raw
In reply to Avi Kivity
Quoted 18 lines · Avi Kivity
Quoted 4 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. Other tasks can become eligible, hopefully the task holding the spinlock, and by the time we're scheduled back the long running task will have finished and released the lock.

Quoted 5 lines

For newer Linux as a guest we're better off paravirtualizing this, so we can tell the host which vcpu holds the lock; in this case kvm will want to say, take a couple milliseconds off my account and transfer it to this task (so called directed yield). However there's no reason to paravirtualize all cpu_relax() calls.

So we're now officially giving up on (soft) realtime virtualization?

#19 lore raw
Quoted 6 lines · Peter Zijlstra
Quoted 3 lines · Avi Kivity
Quoted 3 lines

I.e. this is a somewhat poor solution as far as scheduling goes. But i'm wondering what the CPU side does. Can REP-NOP really take thousands of cycles? If yes, under what circumstances?

Quoted 10 lines
Quoted 6 lines

The guest is running rep-nop in a loop while trying to acquire a spinlock. The hardware detects this (most likely, repeated rep-nop with the same rip) and exits. We can program the loop count; obviously if we're spinning for only a short while it's better to keep spinning while hoping the lock will be released soon.

The idea is to detect that the guest is not making forward progress and yield. If I could tell the scheduler, you may charge me a couple of milliseconds, I promise not to sue, that would be ideal. Other tasks can become eligible, hopefully the task holding the spinlock, and by the time we're scheduled back the long running task will have finished and released the lock.

For newer Linux as a guest we're better off paravirtualizing this, so we can tell the host which vcpu holds the lock; in this case kvm will want to say, take a couple milliseconds off my account and transfer it to this task (so called directed yield). However there's no reason to paravirtualize all cpu_relax() calls.

So we're now officially giving up on (soft) realtime virtualization?

Wouldn't realtime guests be in a realtime scheduling class? That ought to ignore this time_yield() (or however it is eventually named).

Signature · 1 line
error compiling committee.c: too many arguments to function
#20 lore raw

PATCH 1/1 Add support for Pause Filtering to AMD SVM

In reply to Mark Langsdorf
Quoted 1 line

From 01813db8627e74018c8cec90df7e345839351f23 Mon Sep 17 00:00:00 2001

From: Mark Langsdorf <[email protected]>
Date: Thu, 7 May 2009 09:44:10 -0500
Subject: [PATCH] Add support for Pause Filtering to AMD SVM

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   17 +++++++++++++++++
 virt/kvm/kvm_main.c        |    2 ++
 3 files changed, 21 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +17 -0
index ef43a18..4279141 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,12 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2094,15 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2139,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
virt/kvm/kvm_main.c +2 -0
index 2b73e19..e2b730d 100644
@@ -710,6 +710,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	kvm_arch_vcpu_load(vcpu, cpu);
 	put_cpu();
 }
+EXPORT_SYMBOL_GPL(vcpu_load);
 
 void vcpu_put(struct kvm_vcpu *vcpu)
 {
@@ -719,6 +720,7 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 	preempt_enable();
 	mutex_unlock(&vcpu->mutex);
 }
+EXPORT_SYMBOL_GPL(vcpu_put);
 
 static void ack_flush(void *_completed)
 {
Signature · 1 line
1.6.0.2
#21 lore raw
Quoted 5 lines · Mark Langsdorf

From 01813db8627e74018c8cec90df7e345839351f23 Mon Sep 17 00:00:00 2001
From: Mark Langsdorf <[email protected]>
Date: Thu, 7 May 2009 09:44:10 -0500
Subject: [PATCH] Add support for Pause Filtering to AMD SVM

What's the differences wrt retry 1?

Quoted 12 lines

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Quoted 5 lines

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

3000.

Quoted 9 lines

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Like I mentioned earlier, I don't think schedule() does anything on CFS.

Try sched_yield(), but set /proc/sys/kernel/sched_compat_yield.

arch/x86/kvm/svm.c static void init_vmcb(struct vcpu_svm *svm) · line 584

Quoted 7 lines
arch/x86/kvm/svm.c +6 -0
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 5000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
+

Here, 5000?

Quoted 7 lines

}

static int svm_vcpu_reset(struct kvm_vcpu *vcpu)

arch/x86/kvm/svm.c +0 -0
@@ -2087,6 +2094,15 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm, · line 2106

Quoted 10 lines
arch/x86/kvm/svm.c +9 -0
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+
 

You don't need to vcpu_put() and vcpu_load(). The scheduler will call them for you if/when it switches tasks.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#22 lore raw
Quoted 1 line

What's the differences wrt retry 1?

I'm using git format-patch as you requested.

Quoted 12 lines
Quoted 12 lines

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Quoted 6 lines
Quoted 4 lines

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

3000.

Thanks, I keep missing that.

Quoted 11 lines
Quoted 6 lines

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Like I mentioned earlier, I don't think schedule() does anything on CFS.

Try sched_yield(), but set /proc/sys/kernel/sched_compat_yield.

Will do.

Quoted 14 lines
Quoted 1 line

+static int pause_interception(struct vcpu_svm *svm, struct

kvm_run *kvm_run)

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm, · line 2105

Quoted 9 lines
arch/x86/kvm/svm.c +8 -0
+{
+	/* Simple yield */
+	vcpu_put(&svm->vcpu);
+	schedule();
+	vcpu_load(&svm->vcpu);
+	return 1;
+}
+

You don't need to vcpu_put() and vcpu_load(). The scheduler will call them for you if/when it switches tasks.

I was waiting for feedback from Ingo on that issue, but I'll try sched_yield() instead.

-Mark Langsdorf
Operating System Research Center
AMD

#23 lore raw

PATCH 1/1 Add support for Pause Filtering to AMD SVM

In reply to Mark Langsdorf
Quoted 1 line

From 67f831e825b64be5dedae9936ff8a60b884959f2 Mon Sep 17 00:00:00 2001

From: [email protected]
Date: Tue, 19 May 2009 07:46:11 -0500
Subject: [PATCH]

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 5000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   13 +++++++++++++
 include/linux/sched.h      |    7 +++++++
 kernel/sched.c             |    5 +++++
 4 files changed, 27 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +13 -0
index ef43a18..86df191 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,11 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 3000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2093,12 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	set_task_delay(current, 1000000);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2135,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
include/linux/sched.h +7 -0
index b4c38bc..683bc65 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int task_cpu(const struct task_struct *p)
 	return task_thread_info(p)->cpu;
 }
 
+extern void set_task_delay(struct task_struct *p, unsigned int delay);
+
+
 extern void set_task_cpu(struct task_struct *p, unsigned int cpu);
 
 #else
@@ -2292,6 +2295,10 @@ static inline unsigned int task_cpu(const struct task_struct *p)
 	return 0;
 }
 
+void set_task_delay(struct task_struct *p, unsigned int delay)
+{
+}
+
 static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
 {
 }
kernel/sched.c +5 -0
index b902e58..3174620 100644
@@ -1947,6 +1947,11 @@ task_hot(struct task_struct *p, u64 now, struct sched_domain *sd)
 	return delta < (s64)sysctl_sched_migration_cost;
 }
 
+void set_task_delay(struct task_struct *p, unsigned int delay)
+{
+	p->se.vruntime += delay;
+}
+EXPORT_SYMBOL(set_task_delay);
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
Signature · 1 line
1.6.0.2
#24 lore raw
Quoted 12 lines · Mark Langsdorf
include/linux/sched.h +2 -0
index b4c38bc..683bc65 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int task_cpu(const struct task_struct *p)
 	return task_thread_info(p)->cpu;
 }
 
+extern void set_task_delay(struct task_struct *p, unsigned int delay);
 
+++ b/kernel/sched.c

include/linux/sched.h task_hot(struct task_struct *p, u64 now, struct sched_domain *sd) · line 1954

Quoted 8 lines
include/linux/sched.h +5 -0
@@ -1947,6 +1947,11 @@ task_hot(struct task_struct *p, u64 now, struct sched_domain *sd)
 	return delta < (s64)sysctl_sched_migration_cost;
 }
 
+void set_task_delay(struct task_struct *p, unsigned int delay)
+{
+	p->se.vruntime += delay;
+}
+EXPORT_SYMBOL(set_task_delay);

vruntime is nice level scaled, so this is broken. Please run this through the scheduler folks, we can get a facility into an isolated brach for Avi to pull, but it needs some thought.

Ingo

maintainer
#25 lore raw
In reply to Mark Langsdorf

kernel/sched.c task_hot(struct task_struct *p, u64 now, struct sched_domain *sd) · line 1954

Quoted 9 lines · Mark Langsdorf
kernel/sched.c +5 -0
@@ -1947,6 +1947,11 @@ task_hot(struct task_struct *p, u64 now, struct sched_domain *sd)
        return delta < (s64)sysctl_sched_migration_cost;
 }
 
+void set_task_delay(struct task_struct *p, unsigned int delay)
+{
+       p->se.vruntime += delay;
+}
+EXPORT_SYMBOL(set_task_delay);

That's broken, you cannot assume that a task is SCHED_OTHER like that.

Furthermore, you cannot simply change vruntime of any odd task, this only works for current. Also, you really need to call schedule() after doing this for it to have any immediate effect.

Also, if you mean delay to be ns, you need to scale it. Furthermore, I would really really want to export this as GPL only (ok, preferably not at all).

That said, I still thoroughly dislike this whole approach.

/*
* Dumb broken yield like interface -- use at your own peril and know
* RT people will hate you.
*
* Like yield, except for SCHED_OTHER/BATCH, where it will give up @ns
* time for the 'good' cause.
*/
void sched_delay_yield(unsigned long ns)

{
	struct task_struct *curr = current;

	if (curr->sched_class == &fair_sched_class) {
		struct sched_entity *se = &curr->se;
		__update_curr(cfs_rq_of(se), se, ns);
		schedule();
		/* XXX: task accounting ? */
	} else
		sched_yield();
}

EXPORT_SYMBOL_GPL(sched_delay_yield);

#26 lore raw
Quoted 2 lines · Peter Zijlstra

That said, I still thoroughly dislike this whole approach.

Can you explain why? We have a thread that has detected that it's spinning. Keeping on spinning is a waste of cpu time. Why not let something else use the cpu?

Signature · 1 line
error compiling committee.c: too many arguments to function
maintainer
#27 lore raw
Quoted 7 lines · Avi Kivity
Quoted 2 lines · Peter Zijlstra

That said, I still thoroughly dislike this whole approach.

Can you explain why? We have a thread that has detected that it's spinning. Keeping on spinning is a waste of cpu time. Why not let something else use the cpu?

Because its a polling interface. I much prefer it if we were to get a wakeup notification when the vcpu holding the lock releases it.

#28 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines
Quoted 3 lines

That said, I still thoroughly dislike this whole approach.

Can you explain why? We have a thread that has detected that it's spinning. Keeping on spinning is a waste of cpu time. Why not let something else use the cpu?

Because its a polling interface. I much prefer it if we were to get a wakeup notification when the vcpu holding the lock releases it.

It's a fully virtualized guest. There's no way to get this without patching the guest kernel.

Signature · 1 line
error compiling committee.c: too many arguments to function
maintainer
#29 lore raw
Quoted 9 lines · Avi Kivity
Quoted 8 lines · Peter Zijlstra
Quoted 7 lines
Quoted 3 lines

That said, I still thoroughly dislike this whole approach.

Can you explain why? We have a thread that has detected that it's spinning. Keeping on spinning is a waste of cpu time. Why not let something else use the cpu?

Quoted 6 lines
Quoted 3 lines

Because its a polling interface. I much prefer it if we were to get a wakeup notification when the vcpu holding the lock releases it.

It's a fully virtualized guest. There's no way to get this without patching the guest kernel.

Yes there is.. virtualized monitor-wait stuff coupled with a monitor-wait based spinlock implementation.

Once we go change silicon, you might as well do it right.

#30 lore raw
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines

It's a fully virtualized guest. There's no way to get this without patching the guest kernel.

Yes there is.. virtualized monitor-wait stuff coupled with a monitor-wait based spinlock implementation.

That only works if the guest uses monitor/mwait. Not all of the guests are under our control. I don't know whether Windows uses monitor/mwait. Further, we don't have timed exits on mwait like we do with pause.

I've also heard that monitor/mwait are very slow and only usable on idle loop stuff.

Quoted 2 lines

Once we go change silicon, you might as well do it right.

None of the major x86 vendors are under my control.

Signature · 1 line
error compiling committee.c: too many arguments to function
maintainer
#31 lore raw
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines

It's a fully virtualized guest. There's no way to get this without patching the guest kernel.

Quoted 8 lines
Quoted 3 lines

Yes there is.. virtualized monitor-wait stuff coupled with a monitor-wait based spinlock implementation.

That only works if the guest uses monitor/mwait. Not all of the guests are under our control. I don't know whether Windows uses monitor/mwait. Further, we don't have timed exits on mwait like we do with pause.

Ugh, you really care about crap like windows?

Quoted 2 lines

I've also heard that monitor/mwait are very slow and only usable on idle loop stuff.

Yeah, current implementations suck, doesn't mean it has to stay that way.

Quoted 4 lines
Quoted 2 lines

Once we go change silicon, you might as well do it right.

None of the major x86 vendors are under my control.

I thought this patch came from AMD, who changed their silicon so 'solve' one of these virt problems.

/me goes hide again, and pretend all of virt doesn't exist :-) Think happy thoughts.

#32 lore raw
Quoted 12 lines · Peter Zijlstra
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

It's a fully virtualized guest. There's no way to get this without patching the guest kernel.

Yes there is.. virtualized monitor-wait stuff coupled with a monitor-wait based spinlock implementation.

That only works if the guest uses monitor/mwait. Not all of the guests are under our control. I don't know whether Windows uses monitor/mwait. Further, we don't have timed exits on mwait like we do with pause.

Ugh, you really care about crap like windows?

Yes, it is used by my users. Either we convince them not to use Windows, or we find a way to support it well.

Quoted 6 lines
Quoted 2 lines

I've also heard that monitor/mwait are very slow and only usable on idle loop stuff.

Yeah, current implementations suck, doesn't mean it has to stay that way.

Well, I'm not speculating on future cpu changes. I'd like to support current and near-future software and hardware, not how it should have been done software running on how it should have been done hardware.

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

Once we go change silicon, you might as well do it right.

None of the major x86 vendors are under my control.

I thought this patch came from AMD, who changed their silicon so 'solve' one of these virt problems.

They changed the silicon to support existing guests. For both Linux and Windows, the pause instruction is the only indication the guest is spinning.

Quoted 3 lines

/me goes hide again, and pretend all of virt doesn't exist :-) Think happy thoughts.

You'll end up running permanently in a guest, with no way out.

Signature · 1 line
error compiling committee.c: too many arguments to function
#33 lore raw
In reply to Peter Zijlstra
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Once we go change silicon, you might as well do it right.

None of the major x86 vendors are under my control.

I thought this patch came from AMD, who changed their silicon so 'solve' one of these virt problems.

Right, and the change we came up with was to provide a method for filtering PAUSEs and then intercepting after a certain number.

It may not be the solution you wanted, but I can't change the silicon design that has the feature at this point.

-Mark Langsdorf
Operating System Research Center
AMD

#34 lore raw
In reply to Mark Langsdorf
Quoted 7 lines · Mark Langsdorf

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

This result is approximately what you got on your previous patch. Did you measure with the new patch? approximately 1% seems to be too low.

Signature · 1 line
error compiling committee.c: too many arguments to function
#35 lore raw

PATCH 1/1 Add support for Pause Filtering to AMD SVM

In reply to Mark Langsdorf

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   13 +++++++++++++
 include/linux/sched.h      |    7 +++++++
 kernel/sched.c             |   18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +13 -0
index ef43a18..dad6c4b 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,11 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 3000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2093,12 @@ static int interrupt_window_interception(struct vcpu_svm *svm,
 	return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
+{
+	sched_delay_yield(1000000);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2135,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
include/linux/sched.h +7 -0
index b4c38bc..9cde585 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int task_cpu(const struct task_struct *p)
 	return task_thread_info(p)->cpu;
 }
 
+extern void sched_delay_yield(unsigned long ns);
+
+
 extern void set_task_cpu(struct task_struct *p, unsigned int cpu);
 
 #else
@@ -2292,6 +2295,10 @@ static inline unsigned int task_cpu(const struct task_struct *p)
 	return 0;
 }
 
+void sched_delay_yield(struct task_struct *p, unsigned int delay)
+{
+}
+
 static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
 {
 }
kernel/sched.c +18 -0
index b902e58..3aed2f6 100644
@@ -1947,6 +1947,24 @@ task_hot(struct task_struct *p, u64 now, struct sched_domain *sd)
 	return delta < (s64)sysctl_sched_migration_cost;
 }
 
+/*
+ * Interface for yielding a thread by delaying it for a known
+ * interval.  Use at your own risk and not with real-time.
+ *
+ * Like yield, except for SCHED_OTHER/BATCH, where it will
+ * give us @ns time for the 'good' cause.
+ */
+void sched_delay_yield(unsigned long ns)
+{
+	struct task_struct *curr = current;
+	if (curr->sched_class == &fair_sched_class) {
+		struct sched_entity *se = &curr->se;
+		__update_curr(cfs_rq_of(se), se, ns);
+		schedule();
+	} else
+		yield();
+}
+EXPORT_SYMBOL_GPL(sched_delay_yield);
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
Signature · 1 line
1.6.0.2
#36 lore raw
Quoted 19 lines · Mark Langsdorf

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Quoted 9 lines

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Please split this into a scheduler patch and a kvm patch.

Signature · 1 line
error compiling committee.c: too many arguments to function
#37 lore raw
In reply to Mark Langsdorf
Quoted 7 lines · On Thursday 21 May 2009 06:25:17 Mark Langsdorf wrote:

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

(dig it from archives...)

Any update for the patch(I mean the scheduler part)? I think people agreed on the approach?

Signature · 2 lines
regards
Yang, Sheng
Quoted 146 lines

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   13 +++++++++++++
 include/linux/sched.h      |    7 +++++++
 kernel/sched.c             |   18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +2 -1
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +12 -0
index ef43a18..dad6c4b 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,11 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 3000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2093,12 @@ static int interrupt_window_interception(struct
 vcpu_svm *svm, return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run
 *kvm_run) +{
+	sched_delay_yield(1000000);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= emulate_on_interception,
@@ -2123,6 +2135,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm
 *svm, [SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= invalid_op_interception,
include/linux/sched.h +7 -0
index b4c38bc..9cde585 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int task_cpu(const struct
 task_struct *p) return task_thread_info(p)->cpu;
 }
 
+extern void sched_delay_yield(unsigned long ns);
+
+
 extern void set_task_cpu(struct task_struct *p, unsigned int cpu);
 
 #else
@@ -2292,6 +2295,10 @@ static inline unsigned int task_cpu(const struct
 task_struct *p) return 0;
 }
 
+void sched_delay_yield(struct task_struct *p, unsigned int delay)
+{
+}
+
 static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
 {
 }
kernel/sched.c +18 -0
index b902e58..3aed2f6 100644
@@ -1947,6 +1947,24 @@ task_hot(struct task_struct *p, u64 now, struct
 sched_domain *sd) return delta < (s64)sysctl_sched_migration_cost;
 }
 
+/*
+ * Interface for yielding a thread by delaying it for a known
+ * interval.  Use at your own risk and not with real-time.
+ *
+ * Like yield, except for SCHED_OTHER/BATCH, where it will
+ * give us @ns time for the 'good' cause.
+ */
+void sched_delay_yield(unsigned long ns)
+{
+	struct task_struct *curr = current;
+	if (curr->sched_class == &fair_sched_class) {
+		struct sched_entity *se = &curr->se;
+		__update_curr(cfs_rq_of(se), se, ns);
+		schedule();
+	} else
+		yield();
+}
+EXPORT_SYMBOL_GPL(sched_delay_yield);
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
#38 lore raw

The last variant of the scheduler that I tried showed worse performance for both the baseline case (no pause filter enabled) and the test case (pause filter enabled) versus not changing the scheduler.

Some other work came up and I haven't have a chance to experiment with this for a while.

-Mark Langsdorf
Operating System Research Center
AMD

Quoted 188 lines

-----Original Message-----
From: Sheng Yang [mailto:[email protected]]
Sent: Wednesday, July 08, 2009 12:20 AM
To: Langsdorf, Mark
Cc: Roedel, Joerg; [email protected]; Ingo Molnar; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH][KVM][retry 4] Add support for Pause Filtering to AMD SVM

Quoted 7 lines · On Thursday 21 May 2009 06:25:17 Mark Langsdorf wrote:

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

(dig it from archives...)

Any update for the patch(I mean the scheduler part)? I think people agreed on
the approach?

Signature · 2 lines
regards
Yang, Sheng

arch/x86/include/asm/svm.h struct __attribute__ ((__packed__))

Quoted 38 lines

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   13 +++++++++++++
 include/linux/sched.h      |    7 +++++++
 kernel/sched.c             |   18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +0 -0
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__)) 

vmcb_control_area {

arch/x86/kvm/svm.c static int

Quoted 34 lines
arch/x86/include/asm/svm.h +2 -1
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +6 -0
index ef43a18..dad6c4b 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,11 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 3000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2093,12 @@ static int 

interrupt_window_interception(struct

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm,

Quoted 12 lines
arch/x86/kvm/svm.c +5 -0
 vcpu_svm *svm, return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run
 *kvm_run) +{
+	sched_delay_yield(1000000);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		= 

emulate_on_interception,

arch/x86/kvm/svm.c static int

Quoted 1 line
arch/x86/kvm/svm.c +0 -0
@@ -2123,6 +2135,7 @@ static int 

(*svm_exit_handlers[])(struct vcpu_svm

arch/x86/kvm/svm.c static int · line 2137

Quoted 3 lines
arch/x86/kvm/svm.c +0 -0
 *svm, [SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = 

emulate_on_interception,

arch/x86/kvm/svm.c static int (*svm_exit_handlers[])(struct vcpu_svm *svm,

Quoted 4 lines
arch/x86/kvm/svm.c +1 -0
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			= 

invalid_op_interception,

include/linux/sched.h static inline unsigned int

Quoted 5 lines
include/linux/sched.h +0 -0
index b4c38bc..9cde585 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int 

task_cpu(const struct

include/linux/sched.h static inline unsigned int

Quoted 10 lines
include/linux/sched.h +3 -0
 task_struct *p) return task_thread_info(p)->cpu;
 }
 
+extern void sched_delay_yield(unsigned long ns);
+
+
 extern void set_task_cpu(struct task_struct *p, unsigned int cpu);
 
 #else
@@ -2292,6 +2295,10 @@ static inline unsigned int 

task_cpu(const struct

include/linux/sched.h static inline unsigned int task_cpu(const struct task_struct *p)

Quoted 8 lines
include/linux/sched.h +4 -0
 task_struct *p) return 0;
 }
 
+void sched_delay_yield(struct task_struct *p, unsigned int delay)
+{
+}
+
 static inline void set_task_cpu(struct task_struct *p, 

unsigned int cpu)

kernel/sched.c task_hot(struct task_struct *p, u64

Quoted 7 lines
include/linux/sched.h +0 -0
 {
 }
kernel/sched.c +0 -0
index b902e58..3aed2f6 100644
@@ -1947,6 +1947,24 @@ task_hot(struct task_struct *p, u64 

now, struct

Quoted 24 lines
kernel/sched.c +18 -0
 sched_domain *sd) return delta < (s64)sysctl_sched_migration_cost;
 }
 
+/*
+ * Interface for yielding a thread by delaying it for a known
+ * interval.  Use at your own risk and not with real-time.
+ *
+ * Like yield, except for SCHED_OTHER/BATCH, where it will
+ * give us @ns time for the 'good' cause.
+ */
+void sched_delay_yield(unsigned long ns)
+{
+	struct task_struct *curr = current;
+	if (curr->sched_class == &fair_sched_class) {
+		struct sched_entity *se = &curr->se;
+		__update_curr(cfs_rq_of(se), se, ns);
+		schedule();
+	} else
+		yield();
+}
+EXPORT_SYMBOL_GPL(sched_delay_yield);
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
#39 lore raw
Quoted 8 lines · Mark

The last variant of the scheduler that I tried showed worse performance for both the baseline case (no pause filter enabled) and the test case (pause filter enabled) versus not changing the scheduler.

Some other work came up and I haven't have a chance to experiment with this for a while.

Um, I am afraid we have the different result... With your scheduler patch, we got 1% more performance improvement in the quick test. Of course more tests are needed to find a better value of delay.

Do you have time to work on it recently? Maybe we can help to push the scheduler part. (oh, as you know, we need to push our PLE...)

Signature · 2 lines
regards
Yang, Sheng
Quoted 207 lines

-Mark Langsdorf
Operating System Research Center
AMD

Quoted 203 lines

-----Original Message-----
From: Sheng Yang [mailto:[email protected]]
Sent: Wednesday, July 08, 2009 12:20 AM
To: Langsdorf, Mark
Cc: Roedel, Joerg; [email protected]; Ingo Molnar; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH][KVM][retry 4] Add support for Pause Filtering to AMD SVM

Quoted 6 lines · On Thursday 21 May 2009 06:25:17 Mark Langsdorf wrote:

This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and intercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

(dig it from archives...)

Any update for the patch(I mean the scheduler part)? I think people agreed on
the approach?

--
regards
Yang, Sheng

arch/x86/include/asm/svm.h struct __attribute__ ((__packed__))

Quoted 38 lines

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly. Perform the reschedule by increasing the the credited time on the VCPU.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

-Mark Langsdorf
Operating System Research Center
AMD

---
 arch/x86/include/asm/svm.h |    3 ++-
 arch/x86/kvm/svm.c         |   13 +++++++++++++
 include/linux/sched.h      |    7 +++++++
 kernel/sched.c             |   18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletions(-)
arch/x86/include/asm/svm.h +0 -0
index 85574b7..1fecb7e 100644
@@ -57,7 +57,8 @@ struct __attribute__ ((__packed__))

vmcb_control_area {

arch/x86/kvm/svm.c static int

Quoted 34 lines
arch/x86/include/asm/svm.h +2 -1
 	u16 intercept_dr_write;
 	u32 intercept_exceptions;
 	u64 intercept;
-	u8 reserved_1[44];
+	u8 reserved_1[42];
+	u16 pause_filter_count;
 	u64 iopm_base_pa;
 	u64 msrpm_base_pa;
 	u64 tsc_offset;
arch/x86/kvm/svm.c +6 -0
index ef43a18..dad6c4b 100644
@@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
 
@@ -575,6 +576,11 @@ static void init_vmcb(struct vcpu_svm *svm)
 
 	svm->nested_vmcb = 0;
 	svm->vcpu.arch.hflags = HF_GIF_MASK;
+
+	if (svm_has(SVM_FEATURE_PAUSE_FILTER)) {
+		control->pause_filter_count = 3000;
+		control->intercept |= (1ULL << INTERCEPT_PAUSE);
+	}
 }
 
 static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
@@ -2087,6 +2093,12 @@ static int

interrupt_window_interception(struct

arch/x86/kvm/svm.c static int interrupt_window_interception(struct vcpu_svm *svm,

Quoted 12 lines
arch/x86/kvm/svm.c +5 -0
 vcpu_svm *svm, return 1;
 }
 
+static int pause_interception(struct vcpu_svm *svm, struct kvm_run
 *kvm_run) +{
+	sched_delay_yield(1000000);
+	return 1;
+}
+
 static int (*svm_exit_handlers[])(struct vcpu_svm *svm,
 				      struct kvm_run *kvm_run) = {
 	[SVM_EXIT_READ_CR0]           		=

emulate_on_interception,

arch/x86/kvm/svm.c static int

Quoted 1 line
arch/x86/kvm/svm.c +0 -0
@@ -2123,6 +2135,7 @@ static int

(*svm_exit_handlers[])(struct vcpu_svm

arch/x86/kvm/svm.c static int · line 2137

Quoted 3 lines
arch/x86/kvm/svm.c +0 -0
 *svm, [SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         =

emulate_on_interception,

arch/x86/kvm/svm.c static int (*svm_exit_handlers[])(struct vcpu_svm *svm,

Quoted 4 lines
arch/x86/kvm/svm.c +1 -0
+	[SVM_EXIT_PAUSE]			= pause_interception,
 	[SVM_EXIT_HLT]				= halt_interception,
 	[SVM_EXIT_INVLPG]			= invlpg_interception,
 	[SVM_EXIT_INVLPGA]			=

invalid_op_interception,

include/linux/sched.h static inline unsigned int

Quoted 5 lines
include/linux/sched.h +0 -0
index b4c38bc..9cde585 100644
@@ -2283,6 +2283,9 @@ static inline unsigned int

task_cpu(const struct

include/linux/sched.h static inline unsigned int

Quoted 10 lines
include/linux/sched.h +3 -0
 task_struct *p) return task_thread_info(p)->cpu;
 }
 
+extern void sched_delay_yield(unsigned long ns);
+
+
 extern void set_task_cpu(struct task_struct *p, unsigned int cpu);
 
 #else
@@ -2292,6 +2295,10 @@ static inline unsigned int

task_cpu(const struct

include/linux/sched.h static inline unsigned int task_cpu(const struct task_struct *p)

Quoted 8 lines
include/linux/sched.h +4 -0
 task_struct *p) return 0;
 }
 
+void sched_delay_yield(struct task_struct *p, unsigned int delay)
+{
+}
+
 static inline void set_task_cpu(struct task_struct *p,

unsigned int cpu)

kernel/sched.c task_hot(struct task_struct *p, u64

Quoted 7 lines
include/linux/sched.h +0 -0
 {
 }
kernel/sched.c +0 -0
index b902e58..3aed2f6 100644
@@ -1947,6 +1947,24 @@ task_hot(struct task_struct *p, u64

now, struct

Quoted 24 lines
kernel/sched.c +18 -0
 sched_domain *sd) return delta < (s64)sysctl_sched_migration_cost;
 }
 
+/*
+ * Interface for yielding a thread by delaying it for a known
+ * interval.  Use at your own risk and not with real-time.
+ *
+ * Like yield, except for SCHED_OTHER/BATCH, where it will
+ * give us @ns time for the 'good' cause.
+ */
+void sched_delay_yield(unsigned long ns)
+{
+	struct task_struct *curr = current;
+	if (curr->sched_class == &fair_sched_class) {
+		struct sched_entity *se = &curr->se;
+		__update_curr(cfs_rq_of(se), se, ns);
+		schedule();
+	} else
+		yield();
+}
+EXPORT_SYMBOL_GPL(sched_delay_yield);
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
#40 lore raw
Quoted 4 lines

Um, I am afraid we have the different result... With your scheduler patch, we got 1% more performance improvement in the quick test. Of course more tests are needed to find a better value of delay.

What was your test case? How many runs did you do? My results had a lot of variance in them.

Quoted 1 line

Do you have time to work on it recently?

I've been working with the former VI guys on some scheduler improvements for Xen, and hope to get back to this some next week.

-Mark Langsdorf
Operating System Research Center
AMD

#41 lore raw

Mark,
Do you have time to push the Linux scheduler changes for Pause Filtering now? It's almost done after your work. If needing any help, pls. let me know.

Thanks,
edwin

Quoted 21 lines · Mark
Quoted 4 lines

Um, I am afraid we have the different result... With your scheduler patch, we got 1% more performance improvement in the quick test. Of course more tests are needed to
find a better value of delay.

What was your test case? How many runs did you do? My results had a lot of variance in them.

Quoted 2 lines

Do you have time to work on it recently?

I've been working with the former VI guys on some scheduler improvements for Xen, and hope to get back to this some next week.

-Mark Langsdorf
Operating System Research Center
AMD

maintainer
#42 lore raw
In reply to Mark Langsdorf
Quoted 22 lines · Mark Langsdorf

commit 6f15c833f56267baf5abdd0fbc90a81489573053

Author: Mark Langsdorf <[email protected]>
Date:   Mon May 4 15:02:38 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and ntercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Quoted 8 lines

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Isn't a much better solution to the spinlock problem a usable monitor-wait implementation?

If we implement virt spinlocks using monitor-wait they don't spin but simply wait in place, the HV could then decide to run someone else.

This is the HV equivalent to futexes.

The only problem with this is that the current hardware has horrid mwait wakeup latencies. If this were (much) improved you don't need such ugly yield hacks like this.

#43 lore raw
Quoted 39 lines · Peter Zijlstra
Quoted 22 lines · Mark Langsdorf

commit 6f15c833f56267baf5abdd0fbc90a81489573053

Author: Mark Langsdorf <[email protected]>
Date:   Mon May 4 15:02:38 2009 -0500

New AMD processors will support the Pause Filter Feature. This feature creates a new field in the VMCB called Pause Filter Count. If Pause Filter Count is greater than 0 and ntercepting PAUSEs is enabled, the processor will increment an internal counter when a PAUSE instruction occurs instead of intercepting. When the internal counter reaches the Pause Filter Count value, a PAUSE intercept will occur.

This feature can be used to detect contended spinlocks, especially when the lock holding VCPU is not scheduled. Rescheduling another VCPU prevents the VCPU seeking the lock from wasting its quantum by spinning idly.

Experimental results show that most spinlocks are held for less than 1000 PAUSE cycles or more than a few thousand. Default the Pause Filter Counter to 3000 to detect the contended spinlocks.

Quoted 8 lines

Processor support for this feature is indicated by a CPUID bit.

On a 24 core system running 4 guests each with 16 VCPUs, this patch improved overall performance of each guest's 32 job kernbench by approximately 1%. Further performance improvement may be possible with a more sophisticated yield algorithm.

Isn't a much better solution to the spinlock problem a usable monitor-wait implementation?

If we implement virt spinlocks using monitor-wait they don't spin but simply wait in place, the HV could then decide to run someone else.

Quoted 5 lines

This is the HV equivalent to futexes.

The only problem with this is that the current hardware has horrid mwait wakeup latencies. If this were (much) improved you don't need such ugly yield hacks like this.

I've considered MWAIT, but its really hard on the hw side:

the hardware would have to generate a 'wakeup', meaning it either has to trap out, or has to send an irq.

Trapping out is only possible on the release-the-lock side - which is usually on the wrong physical CPU, and it also happens _too late_
- such monitor/wait thingies are usually based on MESI cache, and the originating CPU does not wait for everything to happen.

An irq (on the target CPU that notices the cacheline flush) is more feasible, but it is several thousand cycles to begin with.

Irqs/vectors are a lot harder to add in general as well, and incur a cost of several years of CPU-design-cycle latency. Furthermore, the bits around MESI updates are _very_ sensitive codepaths of the CPU, while REP; NOP is a slowpath to begin with.

But ... especially as SMT techniques spread, something like that will have to happen as well - but it will take years. Meanwhile, this particular CPU feature is there, it's fairly intuitive (plus a VM exit doesnt really change the CPU's behavior materially, so a lot easier to validate and get OS support for), so we could use it.

Ingo