Marcelo Tosatti wrote:
On Tue, Jun 02, 2009 at 04:26:11PM +0200, ehrhardt@xxxxxxxxxxxxxxxxxx wrote:
From: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>
[...]
@@ -706,13 +713,19 @@ int kvm_arch_set_memory_region(struct kv
/* request update of sie control block for all available vcpus */
for (i = 0; i < KVM_MAX_VCPUS; ++i) {
- if (kvm->vcpus[i]) {
- if (test_and_set_bit(KVM_REQ_MMU_RELOAD,
- &kvm->vcpus[i]->requests))
- continue;
- kvm_s390_inject_sigp_stop(kvm->vcpus[i],
- ACTION_VCPUREQUEST_ON_STOP);
- }
+ vcpu = kvm->vcpus[i];
+ if (!vcpu)
+ continue;
+
+ if (!test_and_set_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
+ continue;
+
+ if (vcpu->cpu == -1)
+ continue;
What happens if the check for cpu == -1 races with kvm_arch_vcpu_put?
This context will wait until the vcpu_put context is scheduled back in
to clear the bit? Is that OK?
It either comes back to clear the bit or it is consumed on deletion of
the vcpu. Both ways are ok. The question we have to answer is if it
might stall the mem update ioctl for too long.
Because eventually the check for vcpu->cpu == -1 is just an optimization
if we would completely ignore remove it we would have the same problem
-> could it stall the set mem operation too much. That means the "race"
is not an issue it might just be sub-optimal, but the chance for a long
stall could become an issue. Unfortunately I have no better approach to
that (yet), until then this I like this implementation more than what we
would have without all the corner case fixes in that patch series.
+
+ kvm_s390_inject_sigp_stop(vcpu, ACTION_VCPUREQUEST_ON_STOP);
+ wait_on_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD,
+ wait_bit_schedule, TASK_UNINTERRUPTIBLE);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
{
+ vcpu->cpu = -1;
save_fp_regs(&vcpu->arch.guest_fpregs);
[...]
+++ kvm/arch/s390/kvm/kvm-s390.h
@@ -92,6 +92,13 @@ static inline unsigned long kvm_s390_han
if (!vcpu->requests)
return 0;
+ /* requests that can be handled at all levels */
+ if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests)) {
+ smp_mb__after_clear_bit();
Really need that smp_mb__after_clear_bit ? AFAIK test_and_clear_bit
implies a barrier?
Well I agree that practically test_and_clear_bit has a barrier on s390,
but as far as I read Documentation/atomic_ops.txt at line 339-360 I
think the interface does not imply it so I wanted to add it explicitly.
I would be happy if someone really knows the in depth details here and
corrects me :-)
+ wake_up_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD);
+ kvm_s390_vcpu_set_mem(vcpu);
+ }
+
return vcpu->requests;
}
Index: kvm/virt/kvm/kvm_main.c
===================================================================
--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -1682,6 +1682,10 @@ static int kvm_vcpu_release(struct inode
{
struct kvm_vcpu *vcpu = filp->private_data;
+ clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests);
+ smp_mb__after_clear_bit();
+ wake_up_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD);
+
And this should be generic? Say if other architectures want to make use
of a similar wait infrastructure. Talk is cheap.
Clear bit and wake up on release doesn't hurt any architecture, but it
is at a good place fine for those using the mechanism to ensure cleaning
up outstanding things when closing a vcpu fd.
I thought its not worth to add kvm_ARCH_vcpu_release for it while I
could do so if we want it separated.
(continued below)
Anyway, yeah, the set request / wait mechanism you implement here is
quite similar to the idea mentioned earlier that could be used for x86.
Just get rid of this explicit KVM_REQ_MMU_RELOAD knowledge in
arch-independent code please (if you want to see this merged).
I agree to lift the wait part to other archs later if needed, but as
mentioned above I could move this to arch code to the cost of one arch
hook more. But as also mentioned it doesn't really hurt. I agree that it
does not need to be KVM_REQ_MMU_RELOAD specific, we could just
walk/clear/wake all bits on that vcpu->requests variable.
Would that be generic enough in your opinion ?
Later it can all be lifted off to arch independent code.
True for the wait part which can evolve in our arch code until it is
ripe to get cross arch merged.
--
Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
