From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v8 15/17] vmx: VT-d posted-interrupt core logic handling Date: Mon, 26 Oct 2015 15:39:30 +0100 Message-ID: <1445870370.2717.103.camel@citrix.com> References: <1444640103-4685-1-git-send-email-feng.wu@intel.com> <1444640103-4685-16-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1444514662352657948==" Return-path: In-Reply-To: <1444640103-4685-16-git-send-email-feng.wu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Feng Wu , xen-devel@lists.xen.org Cc: George Dunlap , Andrew Cooper , Kevin Tian , Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============1444514662352657948== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-0CNGh5YyzZrYD28nthW1" --=-0CNGh5YyzZrYD28nthW1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hey, Here I am reviewing this patch, sorry for the delay. Ok, we have discussed a lot about all this, and in fact I had to go back in my mail archive and re-read the rather long sub-thread for this patch in v7. :-) Also, in that thread, I found (as I was recalling there being) a couple of = open questions, one even pointing to the possibility of adopting a differen= t design for this part of the code, which I am not sure could have been con= sidered a closed matter. In any case, it would have been nice, given the situation, if you'd have pu= t a few words about, e.g., which solution you ended up implementing and why= , either in the cover or here (e.g., in the '---' area). =46rom the design point of view, I said during v7 that I don't dislike having= some of the things that this feature requires dpme in (VMX specific part o= f) the context switch path, and that is still valid. What I really don't like much is this blocking cancellation hook you have i= ntroduced. I mean... On Mon, 2015-10-12 at 16:55 +0800, Feng Wu wrote: > - Add the following hooks, this part was suggested > by George Dunlap and > Dario Faggioli . > * arch_vcpu_block() > Called alled before vcpu is blocking and update the PID > (posted-interrupt descriptor). >=20 > * arch_vcpu_block_cancel() > Called when interrupts come in during blocking. >=20 ... This one. Reason is, hooks are not, IMO, among the nicest things. You have to remember to call them, you have to put the call to them in the proper place, etc., when writing the code. OTOH, when reading the code, they break the flow and force one to go and figure out what happens in potentially not so related areas. In summary, they're hard to get right. :-/ That being said, I can live with this, but I wonder whether we really can't do without. For instance, Jan said in the v7 thread: "Couldn't this be taken care of by, if necessary, adjusting PI state in vmx_do_resume()?" This is actually what started the sub-sub-thread about the alternative design of doing everything during VMENTERs/VMEXITs. If you are unconvinced about going that path all the way, would at least do the fixup in there (i.e., taking care of the case where we called arch_vcpu_block() but then we did not block) work and make sense? Actually, I think even another possible implementation variant that was suggested at some point (by George, in this case, for other reasons and purposes) could make this adding this hook unnecessary, i.e.: "vcpu_block() set(_VPF_blocked) local_events_need_delivery() hvm_vcpu_has_pending_irq() ... context_switch v->arch.block() - Add v to pcpu.pi_blocked_vcpu - NV =3D> pi_wakeup_vector If we do it [this] way, and an interrupt comes in before the context switch is finished, it will call posted_intr_vector. We can, at that point, check to see if the current vcpu is marked as blocked. If it=20 is, we can call vcpu_unblock() without having to modify NV or worry=20 about adding / removing the vcpu from the pi_blocked_vcpu list." At the time, I "voted against" this design, because it seemed we could manage to handle interrupt ('regular' and posted) happening during blocking in one and unified way, and with _only_ arch_vcpu_block(). If that is no longer the case (and it's not, as we're adding more hooks, and the need to call the second is a special case being introduced by PI), it may be worth reconsidering things... So, all in all, I don't know. As said, I don't like this cancellation hook because it's one more hook and because --while I see why it's useful in this specific case-- I don't like having it in generic code (in schedule.c), and even less having it called in two places (vcpu_block() and do_pool()). However, if others (Jan and George, I guess) are not equally concerned about it, I can live with it. Thoughts? > * vmx_pi_switch_from() > Called before context switch, we update the PID when the > vCPU is preempted or going to sleep. >=20 > * vmx_pi_switch_to() > Called after context switch, we update the PID when the vCPU > is going to run. >=20 > * arch_vcpu_wake_prepare() > It will be called when waking up the vCPU, we update > the posted interrupt descriptor when the vCPU is > unblocked. >=20 The rest of the patch seems fine to me (at least the scheduling related implications). Just a few (pretty minor) comments. > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1608,6 +1608,18 @@ void context_switch(struct vcpu *prev, struct > vcpu *next) > if ( (per_cpu(curr_vcpu, cpu) =3D=3D next) || > (is_idle_domain(nextd) && cpu_online(cpu)) ) > { > + /* > + * When we handle the lazy context switch for the following > + * two scenarios: > + * - Preempted by a tasklet, which uses in an idle context > + * - the prev vcpu is in offline and no new available vcpus > in run queue > + * We don't change the 'SN' bit in posted-interrupt > descriptor, this > + * may incur spurious PI notification events, but since PI > notification > + * event is only sent when 'ON' is clear, and once the PI > notificatoin > + * is sent, ON is set by hardware, so not so many spurious > events and > + * it is not a big deal. > + */ > + > local_irq_enable(); > } > This comment: can't it leave somewhere else, more VMX and/or PI related? I know this is arch code already but, still, if I'm here, I am reading and trying to understand how context switch works, potentially, not being interested in PI at all... And yet I find this doc comment, talking about some SN and ON bits, without even defining what they are and what they mean. :-/ Really, I'm not saying we shouldn't have it. On the contrary, it has some valuable content in it. Can we just find another place where to put it? Also, about the content. The last part, when it talks about spurious interrupts, it says they're not a problem because we won't get that many. I think that someone not very familiar with this things could use being also told that it is ok/safe to get them (i.e., they don't get lost, etc.). There's an email from George that explain this quite well. I'd also be ok with this particular thing going in the patch changelog, rather than in a comment, as far as it is somewhere. > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 3eefed7..383fd62 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -800,10 +802,13 @@ void vcpu_block(void) > =20 > set_bit(_VPF_blocked, &v->pause_flags); > =20 > + arch_vcpu_block(v); > + > This is maybe not so big of a deal but, since we call this pretty early in the blocking path, and _especially_ if we are to keep the cancellation hook, we may want to consider arch_vcpu_block_prepare() (as we did for wake). Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-0CNGh5YyzZrYD28nthW1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEABECAAYFAlYuOyIACgkQk4XaBE3IOsRZagCffWI3TTAq89621v//2iFWqoXa PpcAnj2Skbs0gd3xXeFqZauE/El4hqEz =lZSG -----END PGP SIGNATURE----- --=-0CNGh5YyzZrYD28nthW1-- --===============1444514662352657948== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1444514662352657948==--