[Toybox] Interpreting top -H -O CPU output
enh
enh at google.com
Thu Nov 14 10:23:52 PST 2019
I'm talking about the pid_max problem. See the patch inlined.
On Thu, Nov 14, 2019, 10:21 Dmitry Shmidt <dimitrysh at google.com> wrote:
> On Thu, Nov 14, 2019 at 8:32 AM enh <enh at google.com> wrote:
>
>> On Wed, Feb 6, 2019 at 3:44 PM enh <enh at google.com> wrote:
>> >
>> > On Wed, Feb 6, 2019 at 2:34 PM Rob Landley <rob at landley.net> wrote:
>> > >
>> > > On 2/6/19 12:56 PM, Dmitry Shmidt wrote:
>> > > > Hi Rob,
>> > > >
>> > > > I am using your top tool implementation for Android from
>> > > > toybox project.
>> > > > I am wondering if in the mode where it shows cpu usage
>> > > > per thread, the total usage per task (process) is included in
>> initial
>> > > > ("main") thread?
>> > >
>> > > I don't really use threads much, Elliott provided most of the use
>> cases for that.
>> > >
>> > > > For example: com.google.android.youtube.tv
>> > > > <http://com.google.android.youtube.tv> shows 153% usage
>> > > > for main thread and some more for other threads, like MainWebView -
>> 31%.
>> > > > Is this 31% included in 153% report or not?
>> > >
>> > > It should never report more than 100%, so it sounds like it is
>> combining CPU
>> > > usage from threads into the parent, yes.
>> > >
>> > > Hmmm... Top -H isn't showing TID by default,
>> >
>> > yeah, that seemed a bit weird to me, but it matches what the
>> > traditional implementation did. (though threads aren't as common on
>> > the desktop.)
>> >
>> > interestingly, i notice that our numbers don't add up. on the desktop,
>> > total == running + sleeping, but our sleeping count is a lot lower
>> > than it should be. (the desktop also says "Tasks:" or "Threads:"
>> > depending on whether you supplied -H, and we don't.)
>> >
>> > we also don't do a good job of sizing the PID field on machines with a
>> > large pid_max. this fixes both of those minor issues, but between the
>> > removal of the `const` on the array and the floating point math i
>> > assume you'll want to do this differently :-)
>>
>> any thoughts on how you'd like to fix this so i can send a patch you'd
>> accept? (the bug bankruptcy bot is asking whether i'm actually going
>> to do anything about this bug, which reminded me...)
>>
>
> I am ok with current solution. After we merged:
> commit 168bfe5382c5a5034b7e208b3253f292b24999ec
> Author: Rob Landley <rob at landley.net>
> Date: Sat Mar 2 22:05:00 2019 -0600
>
> Make top -H show TID instead of PID, not collate %CPU into parent
> thread
> (resulting in 400% CPU with 4 threads), and add a couple comments.
>
> It works as we think it should.
>
> > diff --git a/toys/posix/ps.c b/toys/posix/ps.c
>> > index 079bdbd6..50f52b41 100644
>> > --- a/toys/posix/ps.c
>> > +++ b/toys/posix/ps.c
>> > @@ -314,9 +314,9 @@ struct procpid {
>> > struct typography {
>> > char *name, *help;
>> > signed char width, slot;
>> > -} static const typos[] = TAGGED_ARRAY(PS,
>> > +} static /*const*/ typos[] = TAGGED_ARRAY(PS,
>> > // Numbers. (What's in slot[] is what's displayed, sorted
>> numerically.)
>> > - {"PID", "Process ID", 5, SLOT_pid},
>> > + {"PID", "Process ID", 2, SLOT_pid},
>> > {"PPID", "Parent Process ID", 5, SLOT_ppid},
>> > {"PRI", "Priority (dynamic 0 to 139)", 3, SLOT_priority},
>> > {"NI", "Niceness (static 19 to -20)", 3, SLOT_nice},
>> > @@ -1262,6 +1262,16 @@ static void default_ko(char *s, void *fields,
>> char *err,
>> > struct arg_list *arg)
>> > if (x) help_help();
>> > }
>> >
>> > +static void init_pid_width(void)
>> > +{
>> > + FILE *fp = xfopen("/proc/sys/kernel/pid_max", "re");
>> > + int pid_max;
>> > +
>> > + fscanf(fp, "%d", &pid_max);
>> > + fclose(fp);
>> > + typos[0].width = ceil(log10(pid_max));
>> > +}
>> > +
>> > void ps_main(void)
>> > {
>> > char **arg;
>> > @@ -1270,6 +1280,7 @@ void ps_main(void)
>> > int i;
>> >
>> > TT.ticks = sysconf(_SC_CLK_TCK); // units for starttime/uptime
>> > + init_pid_width();
>> >
>> > if (-1 != (i = tty_fd())) {
>> > struct stat st;
>> > @@ -1546,8 +1557,9 @@ static void top_common(
>> > for (i = 0; i<mix.count; i++)
>> > run[1+stridx("RSTZ", *string_field(mix.tb[i], &field))]++;
>> > sprintf(toybuf,
>> > - "Tasks: %d total,%4ld running,%4ld sleeping,%4ld stopped,"
>> > - "%4ld zombie", mix.count, run[1], run[2], run[3], run[4]);
>> > + "%ss: %d total, %3ld running, %3ld sleeping, %3ld stopped,
>> "
>> > + "%3ld zombie", FLAG(H)?"Thread":"Task", mix.count,
>> > + run[1], run[2], run[3], run[4]);
>> > lines = header_line(lines, 0);
>> >
>> > if (readfile("/proc/meminfo", toybuf, sizeof(toybuf))) {
>> > @@ -1697,6 +1709,7 @@ static void top_setup(char *defo, char *defk)
>> > {
>> > TT.ticks = sysconf(_SC_CLK_TCK); // units for starttime/uptime
>> > TT.tty = tty_fd() != -1;
>> > + init_pid_width();
>> >
>> > // Are we doing "batch" output or interactive?
>> > if (FLAG(b)) TT.width = TT.height = 99999;
>> >
>> >
>> > > and top -H -O TID is never showing
>> > > more than one instance of the same PID... until I sort by TID, and
>> then I get a
>> > > bunch of chrome threads under the same PID, each with 1.5% of the
>> CPU. So yeah,
>> > > CPU usage is per process here, not per thread.
>> > >
>> > > I'm trying to cut a release, but let me add that to the todo list for
>> next
>> > > release. (I should try to come up with a better test case because y
>> system's way
>> > > too loaded normally...)
>> > >
>> > > Rob
>> > > _______________________________________________
>> > > Toybox mailing list
>> > > Toybox at lists.landley.net
>> > > http://lists.landley.net/listinfo.cgi/toybox-landley.net
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20191114/2b3b117a/attachment.htm>
More information about the Toybox
mailing list