* [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
@ 2009-07-20 9:30 Jan Kiszka
2009-07-20 19:46 ` Andrea Arcangeli
2009-07-21 0:00 ` Marcelo Tosatti
0 siblings, 2 replies; 8+ messages in thread
From: Jan Kiszka @ 2009-07-20 9:30 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
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;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-20 9:30 [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request Jan Kiszka
@ 2009-07-20 19:46 ` Andrea Arcangeli
2009-07-21 0:00 ` Marcelo Tosatti
1 sibling, 0 replies; 8+ messages in thread
From: Andrea Arcangeli @ 2009-07-20 19:46 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Marcelo Tosatti, kvm-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-20 9:30 [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request Jan Kiszka
2009-07-20 19:46 ` Andrea Arcangeli
@ 2009-07-21 0:00 ` Marcelo Tosatti
2009-07-21 8:24 ` Jan Kiszka
2009-08-03 12:18 ` Avi Kivity
1 sibling, 2 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2009-07-21 0:00 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-21 0:00 ` Marcelo Tosatti
@ 2009-07-21 8:24 ` Jan Kiszka
2009-07-21 17:10 ` Marcelo Tosatti
2009-08-03 12:18 ` Avi Kivity
1 sibling, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2009-07-21 8:24 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-21 8:24 ` Jan Kiszka
@ 2009-07-21 17:10 ` Marcelo Tosatti
2009-07-21 23:29 ` Jan Kiszka
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2009-07-21 17:10 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm-devel
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).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-21 17:10 ` Marcelo Tosatti
@ 2009-07-21 23:29 ` Jan Kiszka
2009-07-21 23:37 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Jan Kiszka @ 2009-07-21 23:29 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-21 23:29 ` Jan Kiszka
@ 2009-07-21 23:37 ` Marcelo Tosatti
0 siblings, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2009-07-21 23:37 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request
2009-07-21 0:00 ` Marcelo Tosatti
2009-07-21 8:24 ` Jan Kiszka
@ 2009-08-03 12:18 ` Avi Kivity
1 sibling, 0 replies; 8+ messages in thread
From: Avi Kivity @ 2009-08-03 12:18 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Jan Kiszka, kvm-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-08-03 12:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-07-20 9:30 [PATCH] kvm: Drop obsolete cpu_get/put in make_all_cpus_request Jan Kiszka
2009-07-20 19:46 ` Andrea Arcangeli
2009-07-21 0:00 ` Marcelo Tosatti
2009-07-21 8:24 ` Jan Kiszka
2009-07-21 17:10 ` Marcelo Tosatti
2009-07-21 23:29 ` Jan Kiszka
2009-07-21 23:37 ` Marcelo Tosatti
2009-08-03 12:18 ` Avi Kivity
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.