[PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Jan Kiszka]

spin_lock disables preemption, so we can simply read the current cpu.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 virt/kvm/kvm_main.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7cd1c10..98e4ec8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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;
 }

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Andrea Arcangeli]

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

On Mon, Jul 20, 2009 at 11:30:12AM +0200, Jan Kiszka wrote:
> spin_lock disables preemption, so we can simply read the current cpu.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  virt/kvm/kvm_main.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7cd1c10..98e4ec8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

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

On Mon, Jul 20, 2009 at 11:30:12AM +0200, Jan Kiszka wrote:
> spin_lock disables preemption, so we can simply read the current cpu.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  virt/kvm/kvm_main.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7cd1c10..98e4ec8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Jan Kiszka]

Marcelo Tosatti wrote:
> 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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Marcelo Tosatti]

On Tue, Jul 21, 2009 at 10:24:08AM +0200, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > 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.

I see.

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

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

Marcelo Tosatti wrote:
> On Tue, Jul 21, 2009 at 10:24:08AM +0200, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>> 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.
> 
> I see.
> 
>> 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).
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Marcelo Tosatti]

On Wed, Jul 22, 2009 at 01:29:24AM +0200, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Tue, Jul 21, 2009 at 10:24:08AM +0200, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> 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.
> > 
> > I see.
> > 
> >> 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).
> > 
> 
> 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

OK then, applied.

Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request

[Avi Kivity]

On 07/21/2009 03:00 AM, Marcelo Tosatti wrote:
> 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:

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

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