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

From: Suravee Suthikulpanit
Date: Tue Apr 05 2016 - 06:08:16 EST


Hi Radim,

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?

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? If so, I agree. Thanks for the good catch.

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()?


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(). The was_running should only be set to 1 if the vcpu is unloaded but has not yet calling halt.

Am I missing your points somehow?

Thanks,
Suravee