From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 03 of 11 v4] xen: sched_credit: when picking, make sure we get an idle one, if any Date: Fri, 15 Mar 2013 11:37:54 +0100 Message-ID: <1363343874.3912.21.camel@Solace> References: <8a9328d89750c7019e02.1363314645@hit-nxdomain.opendns.com> <5142E68102000078000C5CEE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5321012406376341299==" Return-path: In-Reply-To: <5142E68102000078000C5CEE@nat28.tlf.novell.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: Jan Beulich Cc: Marcus Granado , Dan Magenheimer , Ian Campbell , Anil Madhavapeddy , George Dunlap , Andrew Cooper , Juergen Gross , Ian Jackson , Xen-Devel , Matt Wilson , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org --===============5321012406376341299== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-577cVp3rzxercJ5McEP6" --=-577cVp3rzxercJ5McEP6 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On ven, 2013-03-15 at 08:14 +0000, Jan Beulich wrote: > >>> On 15.03.13 at 03:30, Dario Faggioli wrot= e: > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > > --- a/xen/common/sched_credit.c > > +++ b/xen/common/sched_credit.c > > @@ -532,6 +532,18 @@ static int > > if ( vc->processor =3D=3D cpu && IS_RUNQ_IDLE(cpu) ) > > cpumask_set_cpu(cpu, &idlers); > > cpumask_and(&cpus, &cpus, &idlers); > > + > > + /* > > + * It is important that cpu points to an idle processor, if a suit= able > > + * one exists (and we can use cpus to check and, possibly, choose = a new > > + * CPU, as we just &&-ed it with idlers). In fact, if we are on SM= T, and > > + * cpu points to a busy thread with an idle sibling, both the thre= ads > > + * will be considered the same, from the "idleness" calculation po= int > > + * of view", preventing vcpu from being moved to the thread that i= s > > + * actually idle. > > + */ > > + if ( !cpumask_empty(&cpus) && !cpumask_test_cpu(cpu, &cpus) ) >=20 > I think I had asked about this before -=20 > Did you? I don't remember anything like that but, if you did and I did not answer, sorry for that! :-P > what's the point of the > left hand side of the &&? If the mask is empty, the right hand > side will cover that quite well, at much less a price for high > NR_CPUS (or nr_cpu_ids). >=20 And in fact it was not there, but ISTR having to add it because not having it was leading in some very bad Xen crash... If I remember correctly, this is what happens without it. The code looks like this:=20 if ( !cpumask_empty(&cpus) && !cpumask_test_cpu(cpu, &cpus) ) cpu =3D cpumask_cycle(cpu, &cpus); cpumask_clear_cpu(cpu, &cpus); while ( !cpumask_empty(&cpus) ) { ... } ... return cpu; So, what happens if cpus is actually empty? As you say cpumask_test_cpu(cpu,&cpus) will be false, which means cpu is updated with the result of cpumask(cycle(cpu,&cpus)). If I'm reading the code correctly, a cpumask_cycle() on an empty cpumask will give me nr_cpu_ids, which is then what is returned (the while loop is not entered, so nothing more happens to cpu), which makes things explode... Does that make sense? Time has passed since I saw that bugtrace, so it is possible that my memories are not accurate... I surely can try to reproduce it, if you want to see the "smoking gun" :-) Perhaps I can turn the condition into something like this: if ( !cpumask_test_cpu(cpu, &cpus) ) cpu =3D cpumask_empty(&cpus) ? cpu : cpumask_cycle(cpu, &cpus); So that we pay the price less frequently? > The ASSERT() a few lines earlier > could be simplified in similar ways, btw. >=20 You mean this, right? online =3D cpupool_scheduler_cpumask(vc->domain->cpupool); cpumask_and(&cpus, online, vc->cpu_affinity); cpu =3D cpumask_test_cpu(vc->processor, &cpus) ? vc->processor : cpumask_cycle(vc->processor, &cpus); ASSERT( !cpumask_empty(&cpus) && cpumask_test_cpu(cpu, &cpus) ); Not sure. AFAIU the code, the ASSERT() is indeed willing to make sure that cpus did not ended up being empty as a consequence of the cpumask_and(), and that is done together with the cpumask_test_cpu() just to have only one ASSERT() instead of two, but again, I might well be wrong. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-577cVp3rzxercJ5McEP6 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.4.13 (GNU/Linux) iEYEABECAAYFAlFC+gIACgkQk4XaBE3IOsTcLQCeOkjNLB/ePecnfDnnrNeH0m3W zhsAnjG2Vrb5T+2PmqXNdSK/ao5IfzLf =ykuI -----END PGP SIGNATURE----- --=-577cVp3rzxercJ5McEP6-- --===============5321012406376341299== 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 --===============5321012406376341299==--