From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] Xen sched: Fix multiple runqueues in credit2 Date: Thu, 6 Feb 2014 15:20:08 +0100 Message-ID: <1391696408.9917.28.camel@Solace> References: <1391677118-3071-1-git-send-email-jtweaver@hawaii.edu> <52F37D3C0200007800119BDF@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3028610849892071660==" Return-path: In-Reply-To: <52F37D3C0200007800119BDF@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@eu.citrix.com, Justin Weaver , george.dunlap@eu.citrix.com, esb@ics.hawaii.edu, xen-devel@lists.xen.org, henric@hawaii.edu List-Id: xen-devel@lists.xenproject.org --===============3028610849892071660== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-rHbotphQ4Q1F14fOhPix" --=-rHbotphQ4Q1F14fOhPix Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On gio, 2014-02-06 at 11:17 +0000, Jan Beulich wrote: > >>> On 06.02.14 at 09:58, Justin Weaver wrote: > > --- a/xen/common/sched_credit2.c > > +++ b/xen/common/sched_credit2.c > > @@ -85,8 +85,7 @@ > > * to a small value, and a fixed credit is added to everyone. > > * > > * The plan is for all cores that share an L2 will share the same > > - * runqueue. At the moment, there is one global runqueue for all > > - * cores. > > + * runqueue.=20 >=20 > If this is the intention, then ... > > [...] > > ... this is too simplistic: Whether the L2 is shared by all cores on a > socket should be determined, not assumed. >=20 True. However, what we do right now is trying to build one runqueu per socket, by means of cpu_to_socket(), and failing badly, ending up with only one *system wide* runqueue. Personally, I think that fixing this, i.e., keeping using cpu_to_socket(), but in a correct way, and actually building '1 runqueue per socket' would be a reasonable step in the original author's intentions. Of course, in this case, I concur that the comment above needs changing too. Thoughts? > Apart from that keeping the CPU0 special case at the top is pointless > with the cpu0_socket special casing. >=20 Indeed. If going this route, Justin, I think you can reorganize the whole `if (cpu =3D=3D 0)' (not only the else), and get to a more correct an= d readable solution. > As to coding style: Please fix your comments and get the indentation > of the if/else sequence above right (i.e. either use "else if" with no > added indentation, or enclose the inner if/else in figure braces (I'd > personally prefer the former). >=20 Yep, agreed. To be fair, about comments, sched_credit2.c has quite a mixture of commenting style in it, and it's really an hard call to decide which one should be used. Anyway, Justin, if you reorganize the whole `if () else' block, you are probably better off with a big comment describing the whole thing, before the block itself, for which you can use the following style: /* * Long comment... */ 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) --=-rHbotphQ4Q1F14fOhPix 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 iEYEABECAAYFAlLzmhgACgkQk4XaBE3IOsTIdQCff4xlRFfw3gOpdTvyOD/RBVPW cUsAn2S9DSxHkaiAvrfDth4B10yX9Gfe =kNvQ -----END PGP SIGNATURE----- --=-rHbotphQ4Q1F14fOhPix-- --===============3028610849892071660== 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 --===============3028610849892071660==--