2016-04-05 17:07+0700, Suravee Suthikulpanit:
> On 3/31/16 21:19, Radim Krčmář wrote:
>>2016-03-31 15:52+0700, Suravee Suthikulpanit:
>>>On 03/19/2016 04:44 AM, Radim Krčmář wrote:
>>>>2016-03-18 01:09-0500, Suravee Suthikulpanit:
>>>>>+ } else {
>>>>>+ new_entry = READ_ONCE(*entry);
>>>>>+ /**
>>>>>+ * This handles the case when vcpu is scheduled out
>>>>>+ * and has not yet not called blocking. We save the
>>>>>+ * AVIC running flag so that we can restore later.
>>>>>+ */
>>>>
>>>>is_running must be disabled in between ...blocking and ...unblocking,
>>>>because we don't want to miss interrupts and block forever.
>>>>I somehow don't get it from the comment. :)
>>>
>>>Not sure if I understand your concern. However, the is_running bit
>>>setting/clearing should be handled in the avic_set_running below. This part
>>>only handles othe case when the is_running bit still set when calling
>>>vcpu_put (and later on loading some other vcpus). This way, when we are
>>>re-loading this vcpu, we can restore the is_running bit accordingly.
>>
>>I think that the comment is misleading. The saved is_running flag only
>>matters after svm_vcpu_blocking, yet the comment says that it handles
>>the irrelevant case before.
>
> Actually, my understanding is if the svm_vcpu_blocking() is called, the
> is_running bit would have been cleared. At this point, if the vcpu is
> unloaded. We should not need to worry about it. Is that not the case here?
svm_vcpu_blocking() clears is_running so we don't wait infinitely if an
interrupt arrives between kvm_vcpu_check_block() and schedule().
was_running ensures that preempt notifiers don't set is_running between
kvm_vcpu_check_block() and schedule() and it's the only place where we
need to worry about was_running causing a bug.
The comment would be better if it covered the case we actually care
about and I think that we can change was_running to make it clear even
without a comment.
>>Another minor bug is that was_running isn't initialized to 1, so we need
>>to halt before is_running gets set.
>
> Just to make sure, you are referring to the point where the is_running is
> not set for first time the vcpu is loaded?
Yes.
>>It would be clearer to toggle a flag in svm_vcpu_(un)blocking and set
>>is_running = !is_blocking.
>
> Not sure what you meant here. We are already setting/unsetting the
> is_running bit when vcpu is blocking/unblocking. Are you suggesting just
> simply move the current avic_set_running() into the svm_vcpu_blocking and
> svm_vcpu_unblocking()?
No, that would be buggy. (The code needs to force is_running to true on
svm_vcpu_unblocking().)
I meant to change the place where we remember that is_running must not
be true. Something like
svm_vcpu_blocking(struct kvm_vcpu *vcpu):
vcpu->is_blocking = true;
avic_set_running(vcpu, false);
avic_vcpu_load(struct kvm_vcpu *vcpu, bool is_load):
avic_set_running(vcpu, is_load && !vcpu->is_blocking)
>>Doing so will also be immeasurably faster,
>>because avic_vcpu_load is called far more than svm_vcpu_(un)blocking.
>
> Actually, this is not the same as handling normal vcpu blocking and
> unblocking, which we are already setting/un-setting the is_running bit in
> the avic_set_running().
There is no practical difference after fixing the bug where was_running
starts as 0.
> The was_running should only be set to 1 if the vcpu
> is unloaded but has not yet calling halt.
Yes. was_running must be 0 inside of svm_vcpu_blocking and
svm_vcpu_unblocking and should be 1 outside.
> Am I missing your points somehow?
I'm not sure ...
--
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
