From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PART1 RFC v3 12/12] svm: Manage vcpu load/unload when enable AVIC Date: Tue, 5 Apr 2016 16:56:57 +0200 Message-ID: <20160405145656.GA17400@potion.brq.redhat.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> <57038E6B.3080808@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: pbonzini@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com To: Suravee Suthikulpanit Return-path: Content-Disposition: inline In-Reply-To: <57038E6B.3080808@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-04-05 17:07+0700, Suravee Suthikulpanit: > 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. >=20 > Actually, my understanding is if the svm_vcpu_blocking() is called, t= he > 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 n= eed >>to halt before is_running gets set. >=20 > Just to make sure, you are referring to the point where the is_runnin= g 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 =3D !is_blocking. >=20 > Not sure what you meant here. We are already setting/unsetting the > is_running bit when vcpu is blocking/unblocking. Are you suggesting j= ust > 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 o= n 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 =3D 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. >=20 > Actually, this is not the same as handling normal vcpu blocking and > unblocking, which we are already setting/un-setting the is_running bi= t 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 th= e 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 ...