From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 04/10] xen: credit2: only reset credit on reset condition Date: Fri, 30 Sep 2016 14:57:04 +0200 Message-ID: <1475240224.5315.102.camel@citrix.com> References: <147520253247.22544.10673844222866363947.stgit@Solace.fritz.box> <147520402650.22544.2188671255927985759.stgit@Solace.fritz.box> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7934970700931957039==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bpxNh-0003dH-DI for xen-devel@lists.xenproject.org; Fri, 30 Sep 2016 12:57:37 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: anshul makkar , xen-devel@lists.xenproject.org Cc: George Dunlap List-Id: xen-devel@lists.xenproject.org --===============7934970700931957039== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-9qze5yAc8OfWCTJ2c2cz" --=-9qze5yAc8OfWCTJ2c2cz Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-09-30 at 13:25 +0100, anshul makkar wrote: > On 30/09/16 03:53, Dario Faggioli wrote: > > @@ -2336,6 +2345,7 @@ csched2_schedule( > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct csched2_runqueue_data *rqd; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct csched2_vcpu * const scurr =3D CSC= HED2_VCPU(current); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct csched2_vcpu *snext =3D NULL; > > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned int skipped_vcpus =3D 0; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct task_slice ret; > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0SCHED_STAT_CRANK(schedule); > > @@ -2385,7 +2395,7 @@ csched2_schedule( > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0snext =3D CSCHED2= _VCPU(idle_vcpu[cpu]); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0snext =3D runq_candida= te(rqd, scurr, cpu, now); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0snext =3D runq_candida= te(rqd, scurr, cpu, now, > > &skipped_vcpus); > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* If switching from a non-idle runnable = vcpu, put it > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* back on the runqueue. */ > > @@ -2409,8 +2419,21 @@ csched2_schedule( > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0__set_bit(__CSFLAG_scheduled, &snext->flags); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > >=20 > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Check for the reset= condition */ > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( snext->credit <= =3D CSCHED2_CREDIT_RESET ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* The reset cond= ition is "has a scheduler epoch come to > > an end?". > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* The way this i= s enforced is checking whether the vcpu > > at the top > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* of the runqueu= e has negative credits. This means the > > epochs have > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* variable lengh= t, as in one epoch expores when: > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A01) = the vcpu at the top of the runqueue has executed > > for > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0around 10 ms (with default parameters); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A02) = no other vcpu with higher credits wants to run. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Here, where we= want to check for reset, we need to make > > sure the > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* proper vcpu is= being used. In fact, > > runqueue_candidate() may have > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* not returned t= he first vcpu in the runqueue, for > > various reasons > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* (e.g., affinit= y). Only trigger a reset when it does. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( skipped_vcpus =3D= =3D 0 && snext->credit <=3D > > CSCHED2_CREDIT_RESET ) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > Is there no other way of checking that the returned vcpu from=C2=A0 > runqueue_candidate is the first one apart from using "skipped_vcpu"=C2=A0 > extra variable. Looks a bit ugly. > Well, I guess we can try to use list_first_entry() on the runqueue, and compare it with snext. However, that "leaks" the bit of information that the runqueue is implemented as a list, and would break if at some point we decide to use something different. TBF, this is not a big problem, as we're inside the scheduler implementation anyway (so not really too big of a leak), and we'd most likely notice the breakage at build time... but still. More important, I don't think that "just" doing that would work. In fact, if scurr is running, and there is no-one in the runqueue, and scurr's credits are negative, right now, we reset. So, to catch that case the condition would need to become something like this: =C2=A0if ( (list_empty(&RQD(ops, cpu)->runq) || =C2=A0 =C2=A0 =C2=A0 =C2=A0list_first_entry(&RQD(ops, cpu)->runq) =3D=3D sn= ext) && =C2=A0 =C2=A0 =C2=A0 snext->credit <=3D CSCHED2_CREDIT_RESET ) I agree that counting is a bit ugly, but this one here above does not look much prettier to me. Also, the number of skipped vcpus, if reported via tracing, does convey some information on the status of the runqueue. Nothing crucial, but something that could be useful (at least, it was to me, during developing and debugging). So, for these reasons, I'd be inclined to leave this as it is. 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) --=-9qze5yAc8OfWCTJ2c2cz 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 iQIcBAABCAAGBQJX7mEgAAoJEBZCeImluHPu/eQQAKso1zKS4Xi62zGytqjOGk2E xn4nDeLE7U4Ry+AKmLvETla8ZG7ugGf146Puwk9bFYm+962B9+P7DnK6CJiLzB0Y JmpQrn6RScSZY9Ibh24gBONiKvw+VGYDx7bplcQ7EAWCZmdXF7fN2BOYyqM7h6yr hWkhxFjgSN3/mN+fF6Wwl4lUml4Pl0WySeFpzMvAtKOBsT1CAXl4JR2TiUdFx2sw +diWYe4fnnJSutXEVI9jCycO/I631etI15cWU8rBDIPpisKE//8wpJX78asmHz/B tor2Rgj94kN2DQRBa+z6sv13vsyoBiQf+dgVfqU9Mk54kqcQM+0tzJN3KhENmr+f 0kEEoyo1Q19fmj9FAPVSWtQ6S2oO/ohhafY7fI+pmkobL2eFsDGevCv5RBBF/Svh ubnwcJ5p1FXyDt/ZtMSjnLXxnLyhRRDHyjR40URxvnYPk885NwyrmI2SCLrysMrd qhHWX/WUHWd7qziVFfNr46wFVTD/oscp2llyMg6Yc7+VhagbfmUZo41NLNt4aOFN 2zooQ7Xx6gJCH/EgPo6/NXedJf203g7IylMEgz751wm71GMhMblwHVYDu1AC8GqJ SdlUlyoMMWe3ejIaefc7VYlP+EYNtZ+8tknlfeYlTH4GKSjUXK+Zrs7rHZjddYFi F1FFCy04aEAvrovTzpOT =eDrF -----END PGP SIGNATURE----- --=-9qze5yAc8OfWCTJ2c2cz-- --===============7934970700931957039== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============7934970700931957039==--