From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v3 3/6] VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed Date: Tue, 6 Sep 2016 11:21:33 +0200 Message-ID: <1473153693.19612.103.camel@citrix.com> References: <1472615791-8664-1-git-send-email-feng.wu@intel.com> <1472615791-8664-4-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2235958646669090128==" Return-path: In-Reply-To: <1472615791-8664-4-git-send-email-feng.wu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Feng Wu , xen-devel@lists.xen.org Cc: george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, kevin.tian@intel.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============2235958646669090128== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-bxmu32j68OZcW2JlLzwB" --=-bxmu32j68OZcW2JlLzwB Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2016-08-31 at 11:56 +0800, Feng Wu wrote: > We should remove the vCPU from the per-cpu blocking list > if it is going to be destroyed. >=20 > Signed-off-by: Feng Wu > --- > =C2=A0xen/arch/x86/hvm/vmx/vmx.c | 1 + > =C2=A01 file changed, 1 insertion(+) >=20 > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index b869728..37fa2f1 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -346,6 +346,7 @@ static void vmx_vcpu_destroy(struct vcpu *v) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmx_destroy_vmcs(v); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vpmu_destroy(v); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0passive_domain_destroy(v); > +=C2=A0=C2=A0=C2=A0=C2=A0vmx_pi_blocking_cleanup(v); > I'm not too much into VMX, so I may be wrong (in which case, sorry), but is it safe to call this after vmx_destroy_vmcs() ? Also (even if it is), we're basically calling and executing the following (called by vmx_pi_blocking_clanup()): static void vmx_pi_remove_vcpu_from_blocking_list(struct vcpu *v) { =C2=A0=C2=A0=C2=A0=C2=A0unsigned long flags; =C2=A0=C2=A0=C2=A0=C2=A0spinlock_t *pi_blocking_list_lock; =C2=A0=C2=A0=C2=A0=C2=A0struct pi_desc *pi_desc =3D &v->arch.hvm_vmx.pi_des= c; =C2=A0=C2=A0=C2=A0=C2=A0/* =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Set 'NV' field back to posted_intr_vector, = so the =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Posted-Interrupts can be delivered to the v= CPU when =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* it is running in non-root mode. =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ =C2=A0=C2=A0=C2=A0=C2=A0write_atomic(&pi_desc->nv, posted_intr_vector); =C2=A0=C2=A0=C2=A0=C2=A0/* The vCPU is not on any blocking list. */ =C2=A0=C2=A0=C2=A0=C2=A0pi_blocking_list_lock =3D v->arch.hvm_vmx.pi_blocki= ng.lock; =C2=A0=C2=A0=C2=A0=C2=A0/* Prevent the compiler from eliminating the local = variable.*/ =C2=A0=C2=A0=C2=A0=C2=A0smp_rmb(); =C2=A0=C2=A0=C2=A0=C2=A0if ( pi_blocking_list_lock =3D=3D NULL ) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; =C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave(pi_blocking_list_lock, flags); =C2=A0=C2=A0=C2=A0=C2=A0/* =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* v->arch.hvm_vmx.pi_blocking.lock =3D=3D NUL= L here means the vCPU =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* was removed from the blocking list while we= are acquiring the lock. =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ =C2=A0=C2=A0=C2=A0=C2=A0if ( v->arch.hvm_vmx.pi_blocking.lock !=3D NULL ) =C2=A0=C2=A0=C2=A0=C2=A0{ =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(v->arch.hvm_vmx.pi_b= locking.lock =3D=3D pi_blocking_list_lock); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_del(&v->arch.hvm_vmx.p= i_blocking.list); =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0v->arch.hvm_vmx.pi_blocking= .lock =3D NULL; =C2=A0=C2=A0=C2=A0=C2=A0} =C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(pi_blocking_list_lock, flags= ); } Considering that we're destroying, isn't this too much? Maybe it's not a big deal, but I'd have expected that all is needed here is something like: =C2=A0if ( v->arch.hvm_vmx.pi_blocking.lock ) =C2=A0{ =C2=A0 =C2=A0 =C2=A0spin_lock_irqsave(..); =C2=A0 =C2=A0 =C2=A0list_del(..); =C2=A0 =C2=A0 =C2=A0spin_unlock_irqrestore(..); =C2=A0} Maybe the resume, list_remove, and cleanup functions need to be broken up a bit more/better? Also, as a side note (which I think would be more appropriate as a comment to patch 1, but bear with me, I'm just back from vacations, I have a lot of catch up to do, and I'm in hurry! :-P), now that the function is called=C2=A0vmx_pi_remove_vcpu_from_blocking_list(), this comment being part of its body sounds a bit weird: =C2=A0 =C2=A0 ... =C2=A0=C2=A0=C2=A0=C2=A0/* The vCPU is not on any blocking list. */ =C2=A0=C2=A0=C2=A0=C2=A0pi_blocking_list_loc k =3D v->arch.hvm_vmx.pi_blocking.lock; ... I'd take the chance for rephrasing it. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-bxmu32j68OZcW2JlLzwB 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 iQIcBAABCAAGBQJXzoqeAAoJEBZCeImluHPuYb0P/3HoommMObbrcNvViCBIFm+Q 9eJGHE+mCvqNK7qtgHEPmXOFNOBUm8CIE3DUQIxakfED3HbOrIxxHkKTCT6TYTaV 0onrl/tXRkLewPrDqUxdGHwsXFVogKV6R7VmbcZLbFJWiPOwUS++vq31yP5g2Ufk R07Y8v+jHnXS+poJrn9hmwg2JQm0l9H2M+aOPba7a0sunfXmUubZLat6S5c76fm8 nZmjQ4RR95CqNi90pO6Fe8cb6FfsTUYjSO9cb475PmDjdpRdwF4u9VCxn12ekoyu v02hn3xpxEOQ0kE+5s2lzku1nKo4nwNSKErvZRmF60/jszFfGf7wcho+xPKMc+Qz LtG1g+bY98XftGS34wNZoi+gmkAse4lWTuXg/mMcceB6uTSQM62UuTsH+zwMSDsf TqMRiwxHMvPRjxfhXM9ch+NOxRQuaGVrniihQsC+gnucGm1cWYfPbQzrzwFqK91M ZgbNITRVgVUIsBFUBKixDwWikjE8k4sKl71lM23Os545i/ZgzHG0IdJODjIMtO8Q khQYIqiPwI+HrcieXz+9lXA/Or3tHH/oZNgL3MYKgzt6/6OxTwvr6kdIv6gYfuo9 WsiHtlwGfriJc679clI/Qg2uuN9TIT42BX1I32da7fgaNU3dkOtKXowBZMRP1ek5 OwXjkFF6ht8FP85tTYRc =eHj+ -----END PGP SIGNATURE----- --=-bxmu32j68OZcW2JlLzwB-- --===============2235958646669090128== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============2235958646669090128==--