Re: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU is blocked

[Paolo Bonzini <pbonzini@redhat.com>]

On 18/12/2014 04:16, Wu, Feng wrote:
>>> pre-block:
>>> - Add the vCPU to the blocked per-CPU list
>>> - Clear 'SN'
>>
>> Should SN be already clear (and NV set to POSTED_INTR_VECTOR)? 
> 
> I think the SN bit should be clear here, Adding it here is just to make sure
> SN is clear when vCPU is blocked, so it can receive wakeup notification event later.

Then, please, WARN if the SN bit is set inside the if (vcpu->blocked).
Inside that if you can just add the vCPU to the blocked list on vcpu_put.

>> Can it
>> happen that you go from sched-out to blocked without doing a sched-in first?
>>
> 
> I cannot imagine this scenario, can you please be more specific? Thanks a lot!

I cannot either. :)  But it would be the case where SN is not cleared.
So we agree that it cannot happen.

>> In fact, if this is possible, what happens if vcpu->preempted &&
>> vcpu->blocked?
> 
> In fact, vcpu->preempted && vcpu->blocked happens sometimes, but I think there is
> no issues. Please refer to the following case:

I agree that there should be no issues.  But if it can happen, it's better:

1) to separate the handling of preemption and blocking: preemption
handles SN/NV/NDST, blocking handles the wakeup list.

2) to change this

+		} else if (vcpu->blocked) {
+			/*
+			 * The vcpu is blocked on the wait queue.
+			 * Store the blocked vCPU on the list of the
+			 * vcpu->wakeup_cpu, which is the destination
+			 * of the wake-up notification event.

to just

		}
		if (vcpu->blocked) {
			...
		}
> kvm_vcpu_block() 
> 	-> vcpu->blocked = true;
> 	-> prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> 
> 	before schedule() is called, this vcpu is woken up by another guy, so
> 	the state of the vcpu associated thread is changed to TASK_RUNNING,
> 	then preemption happens after interrupts or the following schedule() is
> 	hit, this will call kvm_sched_out(), in which current->state == TASK_RUNNING
> 	and vcpu->preempted is set to true. So now vcpu->preempted and vcpu->blocked
> 	are both true. In vmx_vcpu_put(), we will check vcpu->preempted first, so
> 	the vCPU will not be blocked, and the vcpu->blocked will be set the false in
> 	vmx_vcpu_load().
> 
> 	But maybe I need do a little change to the vmx_vcpu_load() like below:
> 
>                 /*
>                  * Delete the vCPU from the related wakeup queue
>                  * if we are resuming from blocked state
>                  */
>                 if (vcpu->blocked) {
>                         vcpu->blocked = false;
> +						/* if wakeup_cpu == -1, the vcpu is currently not blocked on any
> +						  pCPU, don't need dequeue here */
> +						if (vcpu->wakeup_cpu != -1) {
>                		         spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
>                                 vcpu->wakeup_cpu), flags);
>                     	     list_del(&vcpu->blocked_vcpu_list);
>                         	 spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock,
>                                 vcpu->wakeup_cpu), flags);
>                         	 vcpu->wakeup_cpu = -1;
> +						}
>                 }

Good idea.

Paolo

> Any ideas about this? Thanks a lot!
> 
> Thanks,
> Feng
> 
> 
> 	-> schedule();
> 
> 
>>
>>> - Set 'NV' to POSTED_INTR_WAKEUP_VECTOR
>>>
>>> post-block:
>>> - Remove the vCPU from the per-CPU list
>>
>> Paolo
>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>> --
>> 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