On 12/07/2011 04:18 PM, Marcelo Tosatti wrote:
On Wed, Nov 30, 2011 at 02:29:59PM +0530, Raghavendra K T wrote:+/* + * kvm_pv_kick_cpu_op: Kick a vcpu. + * + * @cpu - vcpu to be kicked. + */ +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int cpu) +{ + struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, cpu); + struct kvm_mp_state mp_state; + + mp_state.mp_state = KVM_MP_STATE_RUNNABLE;Since vcpu->mp_state is not protected by a lock, this is potentially racy. For example: CPU0 CPU1 kvm_pv_kick_cpu_op running vcpuN vcpuN->mp_state = KVM_MP_STATE_RUNNABLE; kvm_emulate_halt vcpuN->mp_state = KVM_MP_STATE_HALTED Is it harmless to lose a kick?
Yes you are right. It was potentially racy and it was harmful too!. I had observed that it was stalling the CPU before I introduced kicked flag.
But now, vcpu->kicked = 1 ==> kvm_make_request(KVM_REQ_UNHALT, vcpu); ==> __vcpu_run() ==> kvm_check_request(KVM_REQ_UNHALT, vcpu) ==> vcpuN->mp_state = KVM_MP_STATE_RUNNABLE; so eventually we will end up in RUNNABLE. Also Avi pointed that, logically kvm_arch_vcpu_ioctl_set_mpstate should be called only in vcpu thread, so after further debugging, I noticed that, setting vcpuN->mp_state = KVM_MP_STATE_RUNNABLE; is not necessary. I 'll remove that in the next patch. Thanks for pointing. -- 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
