Re: [PART1 RFC v3 12/12] svm: Manage vcpu load/unload when enable AVIC

[Radim KrÄmÃÅ]

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 ...