kvm: Drop obsolete cpu_get/put in make_all_cpus_request
PATCH 1/1 kvm: Drop obsolete cpu_get/put in make_all_cpus_request
spin_lock disables preemption, so we can simply read the current cpu.
---
virt/kvm/kvm_main.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +1 -2
@@ -741,8 +741,8 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)if (alloc_cpumask_var (&cpus,GFP_ATOMIC ))cpumask_clear (cpus); - me =get_cpu ();spin_lock (&kvm->requests_lock); + me =smp_processor_id ();kvm_for_each_vcpu (i, vcpu, kvm) {if (test_and_set_bit (req, &vcpu->requests))continue ;
@@ -757,7 +757,6 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)else called = false;spin_unlock (&kvm->requests_lock); -put_cpu ();free_cpumask_var (cpus);return called; }
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
Quoted 34 lines · Jan Kiszka
spin_lockdisables preemption, so we can simply read the current cpu.Signed-off-by Jan Kiszka <[email protected]>---
virt/kvm/kvm_main.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)virt/kvm/kvm_main.c +1
-3@@ -741,8 +741,8 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)if (alloc_cpumask_var (&cpus,GFP_ATOMIC ))cpumask_clear (cpus); - me =get_cpu ();spin_lock (&kvm->requests_lock); + me =smp_processor_id ();kvm_for_each_vcpu (i, vcpu, kvm) {if (test_and_set_bit (req, &vcpu->requests))continue ;@@ -757,7 +757,6 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)else called = false;spin_unlock (&kvm->requests_lock); -put_cpu ();free_cpumask_var (cpus);return called; } --To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Marcelo Tosatti
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
-rt stumbles over both patterns (that's why I stumbled over it in the first place: get_cpu disables preemption, but spin_lock is a sleeping lock under -rt) and actually requires requests_lock to become raw_spinlock_t. Reordering get_cpu and spin_lock would be another option, but not really a gain for both scenarios.
So unless there is a way to make the whole critical section preemptible (thus migration-agnostic), I think we can micro-optimize it like this.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux
Quoted 7 lines · Jan Kiszka
Quoted 5 lines · Marcelo Tosatti
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
Quoted 5 lines
-rt stumbles over both patterns (that's why I stumbled over it in the first place:
get_cpudisables preemption, butspin_lockis a sleeping lock under -rt) and actually requiresrequests_lockto becomeraw_spinlock_t. Reorderingget_cpuandspin_lockwould be another option, but not really a gain for both scenarios.
I see.
Quoted 2 lines
So unless there is a way to make the whole critical section preemptible (thus migration-agnostic), I think we can micro-optimize it like this.
Can't you switch requests_lock to be raw_spinlock_t then? (or whatever is necessary to make it -rt compatible).
Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Jan Kiszka
Quoted 5 lines · Marcelo Tosatti
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
-rt stumbles over both patterns (that's why I stumbled over it in the first place:
get_cpudisables preemption, butspin_lockis a sleeping lock under -rt) and actually requiresrequests_lockto becomeraw_spinlock_t. Reorderingget_cpuandspin_lockwould be another option, but not really a gain for both scenarios.
Quoted 8 lines
I see.
Quoted 2 lines
So unless there is a way to make the whole critical section preemptible (thus migration-agnostic), I think we can micro-optimize it like this.
Can't you switch
requests_lockto beraw_spinlock_tthen? (or whatever is necessary to make it -rt compatible).
raw_spinlock_t over -rt is not comparable to raw_spinlock_t over mainline. So I'm currently carrying a local patch with
#ifdef CONFIG_PREEMPT_RT raw_spinlock_t some_lock; #else spinlock_t some_lock;
#endif
for all locks that need it (there are three ATM).
That said, I'm suspecting there are more problems with kvm over -rt right now. I'm seeing significant latency peeks on the host. Still investigating, though.
However I don't think we should bother too much about -rt compliance in mainline unless the diff is trivial and basically irrelevant for the common non-rt cases.
Jan
Quoted 38 lines · Jan Kiszka
Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Jan Kiszka
Quoted 5 lines · Marcelo Tosatti
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
-rt stumbles over both patterns (that's why I stumbled over it in the first place:
get_cpudisables preemption, butspin_lockis a sleeping lock under -rt) and actually requiresrequests_lockto becomeraw_spinlock_t. Reorderingget_cpuandspin_lockwould be another option, but not really a gain for both scenarios.Quoted 8 lines
I see.
Quoted 2 lines
So unless there is a way to make the whole critical section preemptible (thus migration-agnostic), I think we can micro-optimize it like this.
Can't you switch
requests_lockto beraw_spinlock_tthen? (or whatever is necessary to make it -rt compatible).
raw_spinlock_tover -rt is not comparable toraw_spinlock_tover mainline. So I'm currently carrying a local patch with#ifdef CONFIG_PREEMPT_RT raw_spinlock_t some_lock; #else spinlock_t some_lock;#endif
for all locks that need it (there are three ATM).
That said, I'm suspecting there are more problems with kvm over -rt right now. I'm seeing significant latency peeks on the host. Still investigating, though.
Quoted 5 lines
However I don't think we should bother too much about -rt compliance in mainline unless the diff is trivial and basically irrelevant for the common non-rt cases.
Jan
OK then, applied.
Quoted 7 lines · Marcelo Tosatti
Jan,
This was suggested but we thought it might be safer to keep the get_cpu/put_cpu pair in case -rt kernels require it (which might be bullshit, but nobody verified).
Thinking about it, it is bullshit:
Quoted 5 lines
virt/kvm/kvm_main.c static bool make_all_cpus_request(struct kvm *kvm, unsigned int req) · line 745
Quoted 5 lines
virt/kvm/kvm_main.c +1
-1- me =get_cpu ();spin_lock (&kvm->requests_lock); + me =smp_processor_id ();
The -rt kernel cannot substitute a mutex for the spinlock here, since we are pinned to a single cpu. It will have to use a raw spinlock here.
However, the 'me' variable is completely spurious. It only affects the statistics gathering, I think we can safely drop it.
Signature · 1 line
error compiling committee.c: too many arguments to function
Hi,
I suggested this too first time around when I've seen the patch but they reminded it's needed to make life easier to preempt-rt...
Quoted 34 lines · Jan Kiszka
spin_lockdisables preemption, so we can simply read the current cpu.Signed-off-by Jan Kiszka <[email protected]>---
virt/kvm/kvm_main.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)virt/kvm/kvm_main.c +1
-3@@ -741,8 +741,8 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)if (alloc_cpumask_var (&cpus,GFP_ATOMIC ))cpumask_clear (cpus); - me =get_cpu ();spin_lock (&kvm->requests_lock); + me =smp_processor_id ();kvm_for_each_vcpu (i, vcpu, kvm) {if (test_and_set_bit (req, &vcpu->requests))continue ;@@ -757,7 +757,6 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)else called = false;spin_unlock (&kvm->requests_lock); -put_cpu ();free_cpumask_var (cpus);return called; } --To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html