Re: [PATCH] xen: sched: improve debug dump output.

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4586 bytes --]

On Fri, 2017-01-27 at 12:05 -0500, Meng Xu wrote:
> On Thu, Jan 26, 2017 at 5:08 PM, Dario Faggioli
> <dario.faggioli@citrix.com> 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.
> > 
> > "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.
> > 
> > Any better?
> 
> 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.
> 
> Maybe, something like this:
> "Runqueue 0" change to "Runqueue 0 (Running):"
> "RUNQ" change to "Runqueue 0 (Ready)"
> 
> What do you think?
> 
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:
> >  - the spaces inside '[' ']';
> >  - the big numbers;
> >  - the fact that last_start is rather useless (it's more tracing
> > info
> >    than debug dump info, IMO);
> 
> 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.
> 
That's ok, we're all different... That's what makes the World so
beautiful. :-)

> >  - the fact that the various queues info and CPUs info are not
> >    displaed closer, and they even have "Domain info:" in between
> > them
> >    (which is because of schedule.c, not sched_rt.c, I know);
> >  - the word "info" after "Global RunQueue", "Global DepletedQueue",
> >    "Global Replenishment Events";
> >  - the word "Global", in the names above;
> >  - the onQ and runnable flags being printed, which I don't is
> > really
> >    necessary or useful;
> >  - the lack of scheduler wide information (e.g., the tickled mask,
> > the
> >    next timeout of the replenishment timer, etc);
> > 
> > But this is material for another patch. :-)
> 
> I agree with all of the above output improvements, except for killing
> the last_start info. :-)
> 
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).
> > 
> > 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.
> 
> 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.
> 
Great! So, you'll send your Acked-by soon?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel