From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PART1 RFC v3 12/12] svm: Manage vcpu load/unload when enable AVIC Date: Tue, 5 Apr 2016 17:07:39 +0700 Message-ID: <57038E6B.3080808@amd.com> References: <1458281388-14452-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1458281388-14452-13-git-send-email-Suravee.Suthikulpanit@amd.com> <20160318214450.GB2332@potion.brq.redhat.com> <56FCE556.80306@amd.com> <20160331141908.GA2171@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , , , , To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mail-bl2on0076.outbound.protection.outlook.com ([65.55.169.76]:47232 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757201AbcDEKIK (ORCPT ); Tue, 5 Apr 2016 06:08:10 -0400 In-Reply-To: <20160331141908.GA2171@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Hi Radim, On 3/31/16 21:19, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2016-03-31 15:52+0700, Suravee Suthikulpanit: >> On 03/19/2016 04:44 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>> 2016-03-18 01:09-0500, Suravee Suthikulpanit: >>>> + } else { >>>> + new_entry =3D 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 ...unblockin= g, >>> 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. Th= is part >> only handles othe case when the is_running bit still set when callin= g >> 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 on= ly > 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= =20 is_running bit would have been cleared. At this point, if the vcpu is=20 unloaded. We should not need to worry about it. Is that not the case he= re? > Another minor bug is that was_running isn't initialized to 1, so we n= eed > to halt before is_running gets set. Just to make sure, you are referring to the point where the is_running=20 is not set for first time the vcpu is loaded? If so, I agree. Thanks=20 for the good catch. > It would be clearer to toggle a flag in svm_vcpu_(un)blocking and set > is_running =3D !is_blocking. Not sure what you meant here. We are already setting/unsetting the=20 is_running bit when vcpu is blocking/unblocking. Are you suggesting jus= t=20 simply move the current avic_set_running() into the svm_vcpu_blocking=20 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=20 unblocking, which we are already setting/un-setting the is_running bit=20 in the avic_set_running(). The was_running should only be set to 1 if=20 the vcpu is unloaded but has not yet calling halt. Am I missing your points somehow? Thanks, Suravee