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: Thu, 26 May 2016 19:20:40 +0200 Message-ID: <1464283240.21930.157.camel@citrix.com> References: <1464269954-8056-1-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2894561951757889714==" Return-path: In-Reply-To: <1464269954-8056-1-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: kevin.tian@intel.com, keir@xen.org, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============2894561951757889714== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-lZ3DwjKIi+c4SRyG8VHz" --=-lZ3DwjKIi+c4SRyG8VHz Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Feng, On Thu, 2016-05-26 at 21:39 +0800, Feng Wu wrote: > Feng Wu (4): > =C2=A0 VMX: Properly handle pi when all the assigned devices are removed > =C2=A0 VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed > =C2=A0 VMX: Assign the right value to 'NDST' field in a concern case > =C2=A0 VMX: fixup PI descritpor when cpu is offline >=20 I need some time to carefully look at this series, and I don't have such amount of time right now. I'll try to look at it and send my comments either tomorrow or on Monday. However, allow me to just say that, from a quick glance, the various solutions does not look really convincing to me. Basically, you've added: =C2=A0- a couple of flags (pi_blocking_cleaned_up, down) =C2=A0- a new spinlock (pi_hotplug_lock) And yet, neither the various changelogs, nor code comments explain much about what the lock serializes and protects, and/or what the flags represent ad how they should be used. 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). I'm also non-convinced that, in patch 4, the right thing to do is to just to pick-up a random online pCPU. At some point, during v1 review, I mentioned arch_move_irqs(), because it seemed to me the place from where you actually have the proper information available (i.e., where the vcpu is actually going, e.g. because the pCPU is on is going away!). It may well be the case that I'm wrong about it being suitable, and I'll look better at what you actually have implemented, but at a first glance, it looks cumbersome. 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). Finally, you're calling *per-vCPU* things foo_hotplug_bar, which is rather confusing, as it makes one thinking that they're related to *pCPU* hotplug. Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-lZ3DwjKIi+c4SRyG8VHz 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 iQIcBAABCAAGBQJXRzBpAAoJEBZCeImluHPuHAUP/jFMKbG9H6USnp6fGweB0660 TaVj3C+1WwP6amkY9tNVAoFQC/UGWe/s0yAr7780VlTesxJqS/4jTmtzv0rgyO98 uuHIfiDfPz0w912nH4YEkemfEYjZSnx8QndNwYCbaK2ek/47B2/GJFXEcEaH3xpr vaBk8VnUF8bwJLISFI6utwAWO5qAN6LW78t7I016k795R7dkS0/Fddvd7VRGVrj5 V+RROj9gNpTPFWAxY3s8yEp1xTVdfq8lZwti+1YbTrpY6FUognEoMQ89OLqNul/0 diQiUxdP34Dw28UafGKQj+SWfCdbxX1Hzg/BXkFJJHsxyGYuhKLIg6FnQkkJBQQX LlC76wgIkaSgsAgVVgAr/BI3xS+4sJ80fsb08KmF7qFZdI4uaA75w7W5Ix5u6zTA 3NVSf+p/h+bToSJZbC6F+L5pp18ZOxCZopOY2XLtRnjzMPCGG3TFli9mWMxkqnr/ droOAjb4dhikZ135a+RWVedRBXBPahFj00c6DbFHdzt30MtSsbsZGYUzqbl8oAYE +wBRI3uSAhPleRqMXI8j6FQNXuAZoVkaDUHl1G7FnZpPxnlRl2rPciMSAhdNEqbg PlsY+RXw9EWtW84opU1UsmvpLsSxhbAGn6+eLQGwPLzQB8r1PPGtkxEUpX0tiWIT cvL/aftkm6uVrfzNxweg =K7z9 -----END PGP SIGNATURE----- --=-lZ3DwjKIi+c4SRyG8VHz-- --===============2894561951757889714== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============2894561951757889714==--