| Message ID | 4A643924.6060908@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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
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). -- 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
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
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. -- 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
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.
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; }
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(-) -- 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