From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 0/4] VMX: Properly handle pi descriptor and per-cpu blocking list Date: Wed, 22 Jun 2016 23:33:07 +0200 Message-ID: <1466631187.18398.55.camel@citrix.com> References: <1464269954-8056-1-git-send-email-feng.wu@intel.com> <1464283240.21930.157.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7851220761060348721==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: "Wu, Feng" , "xen-devel@lists.xen.org" Cc: "Tian, Kevin" , "keir@xen.org" , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "jbeulich@suse.com" List-Id: xen-devel@lists.xenproject.org --===============7851220761060348721== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-wOZQpwJHkXzyDLMzrdtF" --=-wOZQpwJHkXzyDLMzrdtF Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2016-05-31 at 10:19 +0000, Wu, Feng wrote: > > -----Original Message----- > > From: Dario Faggioli [mailto:dario.faggioli@citrix.com] > >=C2=A0 > > So, if you want try argumenting a bit on what was your line of > > reasoning when doing things this way, that would be helpful (at > > least > > to me). > 'pi_hotplug_lock' is trying to protect the following scenario: > vmx_pi_blocking_cleanup() gets called either when the last assigned > device is detached or the vCPU is going to be destroyed, and at the > same time vmx_vcpu_block() is running. We need to make sure > after all the blocking vCPU is cleaned up, we should not add new > vCPU to the per-cpu blocking list. And that is why I introduce > ' pi_blocking_cleaned_up' for each vCPU, which being set to > 1 means that we cannot add it to the blocking list in > vmx_vcpu_block(). >=20 By "the vCPU is going to be destroyed", to what code path do you refer? Because, for instance, there's this: =C2=A0=C2=A0domain_kill() --> domain_destroy() --> complete_domain_destroy(= ) -- =C2=A0 -->=C2=A0vcpu_destroy() -->=C2=A0hvm_vcpu_destroy() in which case, the vCPUs are not running --and hence can't block-- during their own destruction. I take that you've found a path where that does not hold, and hence requires this kind of protection? For the other race (last device being unassigned), I'll look more into it, but, in general, I share George's and Jan's views that we need simpler, more consistent and easier to maintain solutions. > For the other flag 'down', it is used for the following scenario: > When a pCPU is going away and meanwhile vmx_vcpu_block() is > called, we should not put the vCPU to a per-cpu blocking list, which > is going away.=C2=A0 >=20 But, in this case, as George basically said already, if a pCPU is being destroyed, there should be no vCPU running on it, and hence no vCPU that, if blocking, would need being added to the pCPU blocking list. > > For instance, now arch_vcpu_block() returns a value and, as you say > > yourself in a comment, that is for (potentially) preventing a vcpu > > to > > block. So the behavior of schedule.c:vcpu_block(), now depends on > > your > > new flag=C2=A0per_cpu(vmx_pi_blocking, v->processor).down. Again, I'll > > look > > better, but this has few chances of being right (at least > > logically). > Like in vcpu_block(),it will check events before actually blocking > the vcpu, > here we just introduce another case in which the vCPU cannot be > blocked. > I don't know why you think this is problematic? >=20 Well, but, right now, it's like this: =C2=A0- the vCPU should block, waiting for an event =C2=A0- it turns out the event is already arrive =C2=A0- we can avoid blocking In your case, AFAICUI, it's: =C2=A0- the vCPU should block, waiting for an event =C2=A0- the event is _not_ arrived, so we indeed should block =C2=A0- we do *not* block, due to some other reason That does not look right to me... what about the fact that we wanted to actually wait for the event? :-O Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-wOZQpwJHkXzyDLMzrdtF 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 v2 iQIcBAABCAAGBQJXawQUAAoJEBZCeImluHPuhzwQAN/M+Amr0ROr6hfp1a0H7Z5c G9xA5xxOAfY7XG9gPnY08MBv3QDeqZ5409OtoFQFg5VlbFp4Z6ZQOmJUkksmyk+r C2d3+ktZHSFOUNe3TELpM8EsCG1+sSXtg8ygCIeHBzNO7oShg75nBaaS+c3/eP3W ro/FsL/Y7Avuj9reEwsgdboiOUs/9LMTdA39zrZxFWOiDZhE4XSTZoljHeRkCiC6 uOh0bGUrWgmcBNavVTyLVGKfQiIFxOk9vDvWpg1kB7qmkAf8SiqG+bsH75pHRGw8 gFcUggc4zrNzxtRPaZEE0NvCDOAveaLAuN2cjclvPyYiz7dWvgnQXwpSgJaOfsI+ IsZhYF4NR5G2PPaLAVwZAAmvWZssPL0VRD98VCCCdmGWX22lUDd9A1/ygvrZ8T6L BWkF0DVXwc2ZKbBpG5gOfO4eSsfuqd0INfA/8sa3ZvVOx94U2kArvaWYmUnhMRI5 V7YIv1aJc8juX5r8qoLSDfuxfBojuQjm/deZ+GEj5prZBm7NC1I1fG1ruUg4YNo1 OMG+qRbXYJuEx+spJXCX+laQ8qiGHpJAnf/SRtrWHfCqlYJjg/qk7ZkS4B2h66yV JJCCqNL2lkNrkEbn0UR7MMFyYALyu9rqfVGy7ByWmZoLA6XTtd2Hadp982DwoX2F BMkSXqQRHc0Vu/YcZBI6 =Eb3J -----END PGP SIGNATURE----- --=-wOZQpwJHkXzyDLMzrdtF-- --===============7851220761060348721== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============7851220761060348721==--