From bde@zeta.org.au Sat Jan 10 21:49:59 2004 Date: Sat, 10 Jan 2004 00:46:36 +1100 (EST) From: Bruce Evans To: Robert Watson Subject: Re: cvs commit: src/sys/kern tty.c > rwatson 2004/01/08 14:49:23 PST > > FreeBSD src repository > > Modified files: > sys/kern tty.c > Log: > Improve the expressiveness of ttyinfo (^T) when dealing with threads > in slightly less usual states: > > If the thread is on a run queue, display "running" if the thread is > actually running, otherwise, "runnable". Sigh, the nested if's in this make unbreaking and demangling of ttyinfo() even more difficult. Rev.1.183 (KSE III) mangled the simple conditional ladder to determine the state string into a horrible if statement with obfuscatory braces, with the help of a bogus check for (td != NULL) (td can't be NULL, and if it could be then the explicit check for it just gives a worse implementation of panic(3) than not checking). Only another couple of terms in the conditional ladder were needed. Rev.1.183 also broke printing of the rss (fixed in rev.1.184). Printing the "*" prefix made the conditional ladder not quite work, and this commit makes it not so simple. Its simplicity could be restored using separate predicates for the 2 TD_ON_RUNQ() and the 2 TD_IS_SLEEPING() cases. This and some some other style bugs and bugs are "fixed" in: %%% Index: tty.c =================================================================== RCS file: /home/ncvs/src/sys/kern/tty.c,v retrieving revision 1.206 diff -u -2 -r1.206 tty.c --- tty.c 8 Jan 2004 22:49:23 -0000 1.206 +++ tty.c 9 Jan 2004 12:48:48 -0000 @@ -2386,10 +2429,10 @@ ttyinfo(struct tty *tp) { - struct proc *p, *pick; struct timeval utime, stime; - const char *stmp, *sprefix; - long ltmp; - int tmp; - struct thread *td; + struct proc *p, *pick; + struct thread *td, *td1; + const char *stateprefix, *state; + long rss; + int load, pctcpu; if (ttycheckoutq(tp,0) == 0) @@ -2397,6 +2440,6 @@ /* Print load average. */ - tmp = (averunnable.ldavg[0] * 100 + FSCALE / 2) >> FSHIFT; - ttyprintf(tp, "load: %d.%02d ", tmp / 100, tmp % 100); + load = (averunnable.ldavg[0] * 100 + FSCALE / 2) >> FSHIFT; + ttyprintf(tp, "load: %d.%02d ", load / 100, load % 100); if (tp->t_session == NULL) @@ -2404,79 +2447,71 @@ else if (tp->t_pgrp == NULL) ttyprintf(tp, "no foreground process group\n"); - else { - PGRP_LOCK(tp->t_pgrp); - if ((p = LIST_FIRST(&tp->t_pgrp->pg_members)) == 0) { - PGRP_UNLOCK(tp->t_pgrp); - ttyprintf(tp, "empty foreground process group\n"); + else if ( ({ PGRP_LOCK(tp->t_pgrp); 0; }), + (p = LIST_FIRST(&tp->t_pgrp->pg_members)) == NULL) { + PGRP_UNLOCK(tp->t_pgrp); + ttyprintf(tp, "empty foreground process group\n"); + } else { + /* + * Pick the most interesting process and copy some of its + * state for printing later. sched_lock must be held for + * most parts of this. Holding it throughout is simplest + * and prevents even unimportant inconsistencies in the + * copy of the state, but may increase interrupt latency + * too much. + */ + mtx_lock_spin(&sched_lock); + for (pick = NULL; p != NULL; p = LIST_NEXT(p, p_pglist)) + if (proc_compare(pick, p)) + pick = p; + PGRP_UNLOCK(tp->t_pgrp); /* XXX lock order inversion? */ + + td = FIRST_THREAD_IN_PROC(pick); /* XXXKSE */ + stateprefix = TD_ON_LOCK(td) ? "*" : ""; /* XXX */ + state = (pick->p_flag & P_SA) ? "KSE" : + TD_ON_RUNQ(td) ? (TD_IS_RUNNING(td) ? + " running" : "runnable") : + TD_IS_SLEEPING(td) ? (TD_ON_SLEEPQ(td) ? + td->td_wmesg : "unknown") : + TD_ON_LOCK(td) ? td->td_lockname : + TD_IS_SUSPENDED(td) ? "suspended" : + TD_AWAITING_INTR(td) ? "intrwait" : + "unknown"; + calcru(pick, &utime, &stime, NULL); + pctcpu = (td->td_kse->ke_pctcpu * 10000 + FSCALE / 2) >> + FSHIFT; + /* + * XXXKSE the rss calculation is bug-for-bug compatible with + * the one in fill_kinfo_proc(). The code for it shouldn't + * be duplicated... + */ + if (pick->p_state == PRS_NEW || pick->p_state == PRS_ZOMBIE || + pick->p_vmspace == NULL) { + rss = 0; } else { - mtx_lock_spin(&sched_lock); - - /* Pick interesting process. */ - for (pick = NULL; p != 0; p = LIST_NEXT(p, p_pglist)) - if (proc_compare(pick, p)) - pick = p; - PGRP_UNLOCK(tp->t_pgrp); - - td = FIRST_THREAD_IN_PROC(pick); - sprefix = ""; - if (pick->p_flag & P_SA) { - stmp = "KSE" ; /* XXXKSE */ - } else { - if (td != NULL) { - if (TD_ON_RUNQ(td)) { - if (TD_IS_RUNNING(td)) - stmp = "running"; - else - stmp = "runnable"; - } else if (TD_IS_SLEEPING(td)) { - if (TD_ON_SLEEPQ(td)) - stmp = td->td_wmesg; - else - stmp = "unknown"; - } else if (TD_ON_LOCK(td)) { - stmp = td->td_lockname; - sprefix = "*"; - } else if (TD_IS_SUSPENDED(td)) { - stmp = "suspended"; - } else if (TD_AWAITING_INTR(td)) { - stmp = "intrwait"; - } else { - stmp = "unknown"; - } - } else { - stmp = "threadless"; - panic("ttyinfo: no thread!?"); - } - } - calcru(pick, &utime, &stime, NULL); - if (pick->p_state == PRS_NEW || - pick->p_state == PRS_ZOMBIE) { - ltmp = 0; - } else { - ltmp = pgtok( - vmspace_resident_count(pick->p_vmspace)); - } - mtx_unlock_spin(&sched_lock); - - ttyprintf(tp, " cmd: %s %d [%s%s] ", pick->p_comm, - pick->p_pid, sprefix, stmp); - - /* Print user time. */ - ttyprintf(tp, "%ld.%02ldu ", - utime.tv_sec, utime.tv_usec / 10000); - - /* Print system time. */ - ttyprintf(tp, "%ld.%02lds ", - (long)stime.tv_sec, stime.tv_usec / 10000); - - /* Print percentage cpu, resident set size. */ - ttyprintf(tp, "%d%% %ldk\n", tmp / 100, ltmp); - - } + rss = vmspace_resident_count(pick->p_vmspace); + if (pick->p_sflag & PS_INMEM) + rss += UAREA_PAGES; + FOREACH_THREAD_IN_PROC(pick, td1) { + if (!TD_IS_SWAPPED(td1)) + rss += td1->td_kstack_pages; + if (td->td_altkstack_obj != NULL) + rss += td->td_altkstack_pages; + } + rss = pgtok(rss); + } + mtx_unlock_spin(&sched_lock); + + /* Print command, pid, state, utime, stime, %cpu and rss. */ + ttyprintf(tp, + " cmd: %s %d [%s%s] %ld.%02ldu %ld.%02lds %d%% %ldk\n", + pick->p_comm, pick->p_pid, stateprefix, state, + (long)utime.tv_sec, utime.tv_usec / 10000, + (long)stime.tv_sec, stime.tv_usec / 10000, + pctcpu / 100, rss); } tp->t_rocount = 0; /* so pending input will be retyped if BS */ } -/* +/*- * Returns 1 if p2 is "better" than p1 * @@ -2494,8 +2529,8 @@ do { \ struct thread *td; \ + \ val = 0; \ FOREACH_THREAD_IN_PROC(p, td) { \ - if (TD_ON_RUNQ(td) || \ - TD_IS_RUNNING(td)) { \ + if (TD_ON_RUNQ(td) || TD_IS_RUNNING(td)) { \ val = 1; \ break; \ @@ -2503,5 +2538,4 @@ } \ } while (0) - #define TESTAB(a, b) ((a)<<1 | (b)) #define ONLYA 2 @@ -2512,17 +2546,16 @@ proc_compare(struct proc *p1, struct proc *p2) { - - int esta, estb; struct ksegrp *kg; + int esta, estb; + mtx_assert(&sched_lock, MA_OWNED); if (p1 == NULL) return (1); - ISRUN(p1, esta); - ISRUN(p2, estb); - /* - * see if at least one of them is runnable + * See if at least one of them is runnable. */ + ISRUN(p1, esta); + ISRUN(p2, estb); switch (TESTAB(esta, estb)) { case ONLYA: @@ -2535,10 +2568,8 @@ */ esta = estb = 0; - FOREACH_KSEGRP_IN_PROC(p1,kg) { + FOREACH_KSEGRP_IN_PROC(p1,kg) esta += kg->kg_estcpu; - } - FOREACH_KSEGRP_IN_PROC(p2,kg) { + FOREACH_KSEGRP_IN_PROC(p2,kg) estb += kg->kg_estcpu; - } if (estb > esta) return (1); %%% Some other style bugs: - locking changes made the diffs relative to old version hard to read. The patch restores the original indentation using a hack that requires gcc. This makes the diff relative to the current version hard to read. Some other bugs: Fixed: - printing of %cpu was broken in rev.1.144 by mismerging one of my previous patches. The initialization of the variable containing %cpu must be moved into the locked region, and reusing `tmp' for this variable is not just bogus; it doesn't work because a single variable can't hold the 2 int values that need to be exported from the locked region. I fixed this partly by renaming the variables. Rev.1.144 fubared it by removing the initialization that gave %cpu instead of moving it. Not fixed: - ttyinfo() prints %cpu (based on ke_pctcpu), but proc_compare() uses kg_estcpu. These are subtly different. I think the difference is not very important here, except ... - ...SCHED_ULE doesn't maintain kg_estcpu, so the comparisions of it in proc_compare() are useless for the SCHED_ULE case. Bruce