From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: sched: improve debug dump output. Date: Fri, 27 Jan 2017 19:36:18 +0100 Message-ID: <1485542178.32103.216.camel@citrix.com> References: <148544957013.26566.1886390191777485188.stgit@Solace.fritz.box> <1485468521.32103.204.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4680355609397571753==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cXBNt-0003zP-CC for xen-devel@lists.xenproject.org; Fri, 27 Jan 2017 18:36:29 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu Cc: George Dunlap , "xen-devel@lists.xenproject.org" , Anshul Makkar List-Id: xen-devel@lists.xenproject.org --===============4680355609397571753== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-2F9JNsGPBRrCK0t4K0JS" --=-2F9JNsGPBRrCK0t4K0JS Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-01-27 at 12:05 -0500, Meng Xu wrote: > On Thu, Jan 26, 2017 at 5:08 PM, Dario Faggioli > wrote: > > "Runqueue 0" tells that what follow is information about the pCPUs > > and > > the vCPUs of that runqueue (it's the same label used above for, > > showing > > more general runqueue information. > >=20 > > "RUNQ:" tells that what follows is the content of the runqueue, > > i.e., > > the vCPUs which are runnable but not running (the one that were > > running > > have been printed already, next to the pCPU they're running on), > > asn > > are waiting for their turn in this runqueue. > >=20 > > Any better? >=20 > I see the relation between RUNQ and Runqueue now. > In RTDS, we did not keep the running VCPUs in the runqueue. > Credit2 does remove the running vCPU from the runqueue too. That's the only (sane?) thing you can do if your runqueue is shared among multiple pCPUs (which is the case for both Credit2 and RTDS). Well, actually, that's indeed the case even in Credit1 (although, there you may leave it in the runq, if you want). > So I feel it may be better to add a short explanation for RUNQ and > Runqueue in the output to make it clearer. >=20 > Maybe, something like this: > "Runqueue 0" change to "Runqueue 0 (Running):" > "RUNQ" change to "Runqueue 0 (Ready)" >=20 > What do you think? >=20 I'll think about it, but I don't terribly feel the need to be this verbose in this kind of output. > > TBH, what I don't especially like in the output above is, within > > the > > vCPU info being printed: > > =C2=A0- the spaces inside '[' ']'; > > =C2=A0- the big numbers; > > =C2=A0- the fact that last_start is rather useless (it's more tracing > > info > > =C2=A0=C2=A0=C2=A0than debug dump info, IMO); >=20 > I feel the last_start is useful, at least to identify the previous > subtle bug in budget accounting. It tells us when the running VCPU > was > scheduled and indicates how the budget will be burned later. > When I saw the last_start is in the previous period and the > burn_budget() still use the old last_start to burn budget for the > current period, I figured out the bug. >=20 That's ok, we're all different... That's what makes the World so beautiful. :-) > > =C2=A0- the fact that the various queues info and CPUs info are not > > =C2=A0=C2=A0=C2=A0displaed closer, and they even have "Domain info:" in= between > > them > > =C2=A0=C2=A0=C2=A0(which is because of schedule.c, not sched_rt.c, I kn= ow); > > =C2=A0- the word "info" after "Global RunQueue", "Global DepletedQueue"= , > > =C2=A0=C2=A0=C2=A0"Global Replenishment Events"; > > =C2=A0- the word "Global", in the names above; > > =C2=A0- the onQ and runnable flags being printed, which I don't is > > really > > =C2=A0=C2=A0=C2=A0necessary or useful; > > =C2=A0- the lack of scheduler wide information (e.g., the tickled mask, > > the > > =C2=A0=C2=A0=C2=A0next timeout of the replenishment timer, etc); > >=20 > > But this is material for another patch. :-) >=20 > I agree with all of the above output improvements, except for killing > the last_start info. :-) >=20 Ok. I'm not yet working on a patch that does remove it. If/when I will and send it, you're more than entitled, and have the necessary power, to Nack it. ;-P > > Going back to printing "idle" or not, also remember that this is > > debug > > output, meant at being mostly useful for developers, or with help > > from > > developers. And developers can easily check in the code what having > > just the CPU ID printed means (in case it's not obvious, which I > > think > > it is, or they don't remember). > >=20 > > That being said, it's not that I can't live with the added "idle" > > indication. But I like it less and would prefer not to add it. >=20 > Sure! I was thinking if we should even avoid printing out the idle > CPU > number to make the output more concise on an idle systems. > Yeah, that was also my first doing. But, then, looking at the output, I found it a little bit too obfuscated the info about what pCPUs are actually in use within the scheduler (think of the case where it's not default, and is in a cpupool). So I ended up deciding to leave it there, all of them, idle or not. > After seeing the complete output, I think the current output is fine. >=20 Great! So, you'll send your Acked-by soon? 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) --=-2F9JNsGPBRrCK0t4K0JS 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 iQIcBAABCAAGBQJYi5MjAAoJEBZCeImluHPu/PAQAMHxgjwP5A8rNvomzBYxJQdX 80SUPS+GozIe3qIuN/jQQHimqqAyksD+VWUeq3/xPdVStqpkFW5aN5DsW55gMTov ENdYkgDG8yAq8p5vZMkCMQdijio2hPRoelBSt4cPTxzXNvVtbMZlr01KWroTQG/5 h+fEi/lcSf4PRIY70ELpAFn6bSOFQ639On5jRu+3/1QmWwR0bTSUR+NzE7fUJm38 SqQMKEkCNBri6Glr85Ji7wvN9w2KGshrihcH1Dwt8IY/UEs3Vp5blufa7b6tD4g9 v89QbbckgItiNg74oRtPUYdSx/PECEe9xsH3liJBkQHzbQsuRuAJC7kADgRUjsIr WcaRp1aYQPI93h4krTIleMTDwiSxthqg3w7tr34opCwqXx7sY2pYqQyfpKNxhHxN k22izW+1/EEyouyro432HFUK0355ggazdPUb041s3dAIl1UEouhHn93+C846NLF9 ws6ed1eFBZAbhuKxjPl2anCuHBVA7jIorDXhdDajrOkO95593e7MSQWgFaDorect 0pugrfLyy4JhERoqyFpKwN584gilf9PIG4IC1YIl8g8O6IxL5wdXk9e/WkUJPhze UyiWn4JD2d0oj9pnBWHk1pjmv7GFBTTQFV8snSkVjOBBkVC6Iph8mM/KkV5vyB5c zgNqbjypBW6DOi2LQee8 =nUED -----END PGP SIGNATURE----- --=-2F9JNsGPBRrCK0t4K0JS-- --===============4680355609397571753== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============4680355609397571753==--