| Message ID | 20170406202056.18379-7-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 06, 2017 at 10:20:56PM +0200, Radim Krčmář wrote: > We want to have kvm_make_all_cpus_request() to be an optmized version of > > kvm_for_each_vcpu(i, vcpu, kvm) { > kvm_make_request(vcpu, request); > kvm_vcpu_kick(vcpu); > } > > and kvm_vcpu_kick() wakes up the target vcpu. We know which requests do > not need the wake up and use it to optimize the loop. Any reason we don't want kvm_vcpu_kick() to also get the if (!(req & KVM_REQUEST_NO_WAKEUP)) optimization condition? I did some grepping, and don't see any kicks of the requests that have been marked as NO_WAKEUP, so nothing should change by adding it now. But the consistency would be nice for the doc I'm writing. Also, the condition in kvm_vcpu_kick() looks like overkill cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu) How could vcpu->cpu ever be any offline/invalid cpu, other than -1? The condition in kvm_make_all_cpus_request() makes more sense to me cpu != -1 && cpu != me I guess a lot this stuff is planned for a larger requests rework, when kicks get integrated with requests? I'm a bit anxious, though, as it changes how I document stuff now, and even how I approach the ARM series. For example, if kvm_make_request() already integrated kvm_vcpu_kick(), which means also adding the smp_mb__after_atomic(), like kvm_make_all_cpus_request() has, then I wouldn't need to add the smp_mb() to kvm_arch_vcpu_should_kick(). Thanks, drew > > Thanks to that, this patch doesn't change the behavior of current users > (the all don't need the wake up) and only prepares for future where the > wake up is going to be needed. > > I think that most requests do not need the wake up, so we would flip the > bit then. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > virt/kvm/kvm_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index a486c6ad27a6..1db503bab3dc 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) > /* Set ->requests bit before we read ->mode. */ > smp_mb__after_atomic(); > > + if (!(req & KVM_REQUEST_NO_WAKEUP)) > + kvm_vcpu_wake_up(vcpu); > + > if (cpus != NULL && cpu != -1 && cpu != me && > kvm_arch_vcpu_should_kick(vcpu)) > cpumask_set_cpu(cpu, cpus); > -- > 2.12.0 >
On 10/04/2017 19:14, Andrew Jones wrote: > Any reason we don't want kvm_vcpu_kick() to also get the > if (!(req & KVM_REQUEST_NO_WAKEUP)) optimization condition? Because what we want is kvm_make_request to do the kick instead, "if (!(req & KVM_REQUEST_NO_WAKEUP))", I think. > I did some > grepping, and don't see any kicks of the requests that have been marked as > NO_WAKEUP, so nothing should change by adding it now. But the consistency > would be nice for the doc I'm writing. > > Also, the condition in kvm_vcpu_kick() looks like overkill > > cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu) > > How could vcpu->cpu ever be any offline/invalid cpu, other than -1? The > condition in kvm_make_all_cpus_request() makes more sense to me > > cpu != -1 && cpu != me > > I guess a lot this stuff is planned for a larger requests rework, when > kicks get integrated with requests? Yes, this is more or less what I meant above. > I'm a bit anxious, though, as it > changes how I document stuff now, and even how I approach the ARM series. > For example, if kvm_make_request() already integrated kvm_vcpu_kick(), > which means also adding the smp_mb__after_atomic(), like > kvm_make_all_cpus_request() has, then I wouldn't need to add the smp_mb() > to kvm_arch_vcpu_should_kick(). kvm_arch_vcpu_should_kick() does cmpxchg, which already includes a memory barrier when it succeeds, so you need not add smp_mb() there. And indeed by integrating kicks and requests we know that all callers of kvm_arch_vcpu_should_kick() already do an atomic + smp_mb__after_atomic(), so there's even less reason to worry about memory barriers. kvm_arch_vcpu_should_kick() could then use cmpxchg_relaxed if it helps ARM, and you could even split the loop in two to limit the number of memory barriers: kvm_for_each_vcpu(i, vcpu, kvm) { set_bit(req & KVM_REQUEST_MASK, &vcpu->requests); smp_mb__after_atomic(); /* now kick and/or wakeup */ It won't make a difference in practice because there's something wrong if kvm_make_all_cpus_request is a hot spot, but it's readable code and it makes sense. In any case, as soon as your patches get in, whoever does the cleanup also has the honor of updating the docs. Radim could also get extra karma for putting your documentation at the beginning of this series, and updating it at the same time. :) Paolo
On 07/04/2017 04:20, Radim Krčmář wrote: > I think that most requests do not need the wake up, so we would flip the > bit then. True. I may need a bit more convincing, but let's see the patches: - point against: on the other hand no wakeup is a bug, possibly hard to find, while an extra wakeup is just annoying. - point in favor: the same argument (multiplied by 9000) would apply to a wait flag in the request number, but it would be obviously stupid to add a no_wait flag to all requests except the couple that need it. Thanks, Paolo
On 07/04/2017 04:20, Radim Krčmář wrote: > I think that most requests do not need the wake up, so we would flip the > bit then. True. I may need a bit more convincing, but let's see the patches: - point against: no wakeup is a bug, possibly hard to find, while an extra wakeup is just annoying. - point in favor: the same argument (multiplied by over 9000) would apply to a wait flag in the request number, but it would be obviously stupid to add a no_wait flag to all requests except the couple that need it. Thanks, Paolo
On Tue, Apr 11, 2017 at 01:34:49PM +0800, Paolo Bonzini wrote: > kvm_arch_vcpu_should_kick() does cmpxchg, which already includes a > memory barrier when it succeeds, so you need not add smp_mb() there. When the cmpxchg() fails it only guarantees ACQUIRE semantics, meaning the request setting may appear to happen after its completion. This would break our delicate vcpu->requests, vcpu->mode two-variable memory barrier pattern that prohibits a VCPU entering guest mode with a pending request and no IPI. IOW, on ARM we need an explicit smp_mb() before the cmpxchg(), otherwise it's incomplete. I think adding a smp_mb__before_atomic() should cover ARM and any other relaxed memory model arches without impacting x86. Thanks, drew
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index a486c6ad27a6..1db503bab3dc 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -186,6 +186,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) /* Set ->requests bit before we read ->mode. */ smp_mb__after_atomic(); + if (!(req & KVM_REQUEST_NO_WAKEUP)) + kvm_vcpu_wake_up(vcpu); + if (cpus != NULL && cpu != -1 && cpu != me && kvm_arch_vcpu_should_kick(vcpu)) cpumask_set_cpu(cpu, cpus);
We want to have kvm_make_all_cpus_request() to be an optmized version of kvm_for_each_vcpu(i, vcpu, kvm) { kvm_make_request(vcpu, request); kvm_vcpu_kick(vcpu); } and kvm_vcpu_kick() wakes up the target vcpu. We know which requests do not need the wake up and use it to optimize the loop. Thanks to that, this patch doesn't change the behavior of current users (the all don't need the wake up) and only prepares for future where the wake up is going to be needed. I think that most requests do not need the wake up, so we would flip the bit then. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- virt/kvm/kvm_main.c | 3 +++ 1 file changed, 3 insertions(+)