From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 2/3] xen: Have schedulers revise initial placement Date: Tue, 26 Jul 2016 11:30:13 +0200 Message-ID: <1469525413.32102.12.camel@citrix.com> References: <1469460493-18842-1-git-send-email-george.dunlap@citrix.com> <1469460493-18842-2-git-send-email-george.dunlap@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0871627065537874927==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bRygy-0003p0-62 for xen-devel@lists.xenproject.org; Tue, 26 Jul 2016 09:30:24 +0000 In-Reply-To: <1469460493-18842-2-git-send-email-george.dunlap@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap , xen-devel@lists.xenproject.org Cc: Anshul Makkar , Meng Xu , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============0871627065537874927== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-JI6QFJGSVmRdCl7mXb6Y" --=-JI6QFJGSVmRdCl7mXb6Y Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2016-07-25 at 16:28 +0100, George Dunlap wrote: > The generic domain creation logic in > xen/common/domctl.c:default_vcpu0_location() attempts to try to do > initial placement load-balancing by placing vcpu 0 on the least-busy > non-primary hyperthread available.=C2=A0=C2=A0Unfortunately, the logic ca= n end > up picking a pcpu that's not in the online mask.=C2=A0=C2=A0When this is = passed > to a scheduler such which assumes that the initial assignment is > valid, it causes a null pointer dereference looking up the runqueue. >=20 Looking more at Credit2 code, I think there are a couple of missing checks that a cpu that is about to be used for something, is actually in online. However, that is orthogonal with this patch and... > Furthermore, this initial placement doesn't take into account hard or > soft affinity, or any scheduler-specific knowledge (such as historic > runqueue load, as in credit2). >=20 ... using pick_cpu() here is, IMO, a really really really good idea, so I think this patch should go in (and I'll work on and, if I am right, add the missing checks). > Signed-off-by: George Dunlap > --- > v2: > - Actually grab lock before calling vcpu_schedule_lock() to avoid > =C2=A0 tripping over a new ASSERT >=20 Ah, yes... sorry! :-P Just one thing: > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -2055,12 +2055,21 @@ csched2_vcpu_insert(const struct scheduler > *ops, struct vcpu *vc) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!is_idle_vcpu(vc)); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(list_empty(&svc->runq_elem)); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0/* Add vcpu to runqueue of initial processor */ > +=C2=A0=C2=A0=C2=A0=C2=A0/* csched2_cpu_pick() expects the pcpu lock to b= e held */ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0lock =3D vcpu_schedule_lock_irq(vc); > =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0vc->processor =3D csched2_cpu_pick(ops, vc); > + > +=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irq(lock); > + > +=C2=A0=C2=A0=C2=A0=C2=A0lock =3D vcpu_schedule_lock_irq(vc); > + > +=C2=A0=C2=A0=C2=A0=C2=A0/* Add vcpu to runqueue of initial processor */ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0runq_assign(ops, vc); > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vcpu_schedule_unlock_irq(lock, vc); > +=C2=A0=C2=A0=C2=A0=C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0local_irq_enable(); >=20 This local_irq_enable() is not necessary any longer, is it? With that off (and I'd be fine if you want to do that while committing): Reviwed-by: Dario Faggioli Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-JI6QFJGSVmRdCl7mXb6Y 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 iQIcBAABCAAGBQJXly2mAAoJEBZCeImluHPuGiUQAO0p+UJXWiZEfFdAQSD78eho 4zadqZFyg+rpkAsOOBSsEz8yvT19NSW8ZVz7ds0RFptm9FRNTo4AxzfIOCx9CMEB tHG1SKG/r1psTgekH1s38KHBS71KJb8tR60wkPvYRbuwEa2udIRRGGtBXp1II5Sx ro7+7nj0SIlWm+4xaypL2hL1DQhVAPGeZFact5bDfn4GiC/6JCoNu/VS8XslsSmM qWIQKuzsMJvxkAs8HkyhUjaLqzrEJXPbAYZSnx8ci7Il1j9SG99WIE80D3gmh4AT EnzLbPxb4+DQWYZcpQcjdpy8dv7VnplKdjjUWsnc0yMVV2ccp8RQ6VREFVtLCevP 9SnJK4HMTsXelrfj8/Bf2WBzaEeEMskejV87KcnOdRm/2wBknasr/23b149U9AUl e8IGQgVt9j6US2lWtTJ97XPAEU2uSrHEHycXM3Lkzd9IsAh5nuf5JIQGsXM35QMg yx9TuK6KzgPshSLGjEdDOV7J2WVe+8C5OCFuF6DpfzBQMUTwGrhjtraTh4HeTyNs jWAqHpG+tkWpE6OrOB/8RelsKK9UuN636ny+lXTLEtL6yyuz/HLA8OnoMHGhZ4jt 9MQcH67QYrLTREti3mhMNtien2l7w7ID6Zyz+CRTN1/yFtZuqq80Dzr64pgJaQ1Q JQg3mp06XSNOP57ARB/8 =3qoG -----END PGP SIGNATURE----- --=-JI6QFJGSVmRdCl7mXb6Y-- --===============0871627065537874927== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============0871627065537874927==--