Re: Updated procstat(1)
On Nov 28, 2007, at 5:46 AM, Robert Watson wrote: On Wed, 28 Nov 2007, Skip Ford wrote: Skip Ford wrote: Robert Watson wrote: On Wed, 28 Nov 2007, Skip Ford wrote: - "-a" now means "all processes", Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a per-process tool. I was, but then I read your e-mail and became convinced that the first patch that would be submitted against procstat(1) would be a "-a" patch. :-) Yep, would've happened. Now the first patch submitted will be a "- w interval" patch... :-) I couldn't resist implementing a crude interval arg just for kicks. Here's the output of find(1) every second. This is so cool: Very neat :-). If you like this, you'll love DTrace, which allows you to do all sorts of things along these lines. I'll add a -w mode, but be aware that if you want to do the below, what you really want is DTrace :-), which allows you do do things like sample kernel stack traces on the clock timer, based on function invocations, etc, so you can do things like say "sample all the paths to a particular kernel function". Now that John is updating DTrace again, I hope that we'll be seeing it in the 8-CURRENT source RSN. Robert N M Watson Computer Laboratory University of Cambridge Sorry, just a bit off topic Have the licensing issues been resolved with regards to DTrace? This is a feature I was looking forward to in 7.0-RELEASE but it had been delayed because of the licensing. Bert JW Regeer
Re: Updated procstat(1)
On Wed, 28 Nov 2007, Bert JW Regeer wrote: Have the licensing issues been resolved with regards to DTrace? This is a feature I was looking forward to in 7.0-RELEASE but it had been delayed because of the licensing. The problems had to do with non-alignment of the licensing vs. software boundaries, and I believe have been addressed by moving the boundaries a bit (i.e., making some more DTrace data structures opaque, etc). The key point is that the CDDL parts will be compartmentalized as we do for other licenses, but that DTrace will still be loadable as a module with a GENERIC kernel, as is the case with ZFS already. Unfortunately, DTrace won't ship in 7.0, but we believe that it can be MFC'd to RELENG_7. I've not checked in with John Birrell in a few days, but when I last checked he was in the throes of updating the code and cleaning up the integration of the Solaris parts, so my hope is that we'll see CVS progress soon. I know a lot of people are very eager to see this happen. Robert N M Watson Computer Laboratory University of Cambridge ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
On Tue, Nov 27, 2007 at 05:18:47PM +, Robert Watson wrote: > The last of these required new kernel changes, including an MD component. > I've tested the MD parts only on i386, although I have quick hacks at what > they should look like on amd64, arm, powerpc, sparc64, sun4v. I don't > promise these compile or work, but they might do. The kernel build didn't work on AMD64... cc -c -O2 -frename-registers -pipe -fno-strict-aliasing -std=c99 -g -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Winline -Wcast-qual -Wundef -Wno-pointer-sign -fformat-extensions -nostdinc -I. -I/usr/src/sys -I/usr/src/sys/contrib/altq -D_KERNEL -DHAVE_KERNEL_OPTION_HEADERS -include opt_global.h -fno-common -finline-limit=8000 --param inline-unit-growth=100 --param large-function-growth=1000 -fno-omit-frame-pointer -mcmodel=kernel -mno-red-zone -mfpmath=387 -mno-sse -mno-sse2 -mno-mmx -mno-3dnow -msoft-float -fno-asynchronous-unwind-tables -ffreestanding -Werror /usr/src/sys/amd64/amd64/db_trace.c cc1: warnings being treated as errors /usr/src/sys/amd64/amd64/db_trace.c: In function 'stack_save_td': /usr/src/sys/amd64/amd64/db_trace.c:535: warning: type defaults to 'int' in declaration of 'rbp' /usr/src/sys/amd64/amd64/db_trace.c:537: warning: implicit declaration of function 'TD_IS_SWWAPPED' /usr/src/sys/amd64/amd64/db_trace.c:537: warning: nested extern declaration of 'TD_IS_SWWAPPED' *** Error code 1 Stop in /usr/obj/usr/src/sys/GENERIC. *** Error code 1 Stop in /usr/src. *** Error code 1 Stop in /usr/src. [EMAIL PROTECTED] /usr/src % Here's an updated patch to sys/amd64/amd64/db_trace.c (it's a diff against revision 1.81). It changes "register rbp" to be "register_t rbp" and fixes the extra "W" in TD_IS_SWAPPED. The kernel built fine after these changes. I'll test it out tomorrow. --- sys/amd64/amd64/db_trace.c.orig 2007-11-15 17:00:56.0 -0500 +++ sys/amd64/amd64/db_trace.c 2007-11-27 22:59:29.0 -0500 @@ -505,15 +505,13 @@ ctx->pcb_rip, count)); } -void -stack_save(struct stack *st) +static void +stack_capture(struct stack *st, register_t rbp) { struct amd64_frame *frame; vm_offset_t callpc; - register_t rbp; stack_zero(st); - __asm __volatile("movq %%rbp,%0" : "=r" (rbp)); frame = (struct amd64_frame *)rbp; while (1) { if (!INKERNEL((long)frame)) @@ -531,6 +529,29 @@ } } +void +stack_save_td(struct stack *st, struct thread *td) +{ + register_t rbp; + + if (TD_IS_SWAPPED(td)) + panic("stack_save_td: swapped"); + if (TD_IS_RUNNING(td)) + panic("stack_save_td: running"); + + rbp = td->td_pcb->pcb_rbp; + stack_capture(st, rbp); +} + +void +stack_save(struct stack *st) +{ + register_t rbp; + + __asm __volatile("movq %%rbp,%0" : "=r" (rbp)); + stack_capture(st, rbp); +} + int amd64_set_watch(watchnum, watchaddr, size, access, d) int watchnum; > I think procstat(1) is getting a lot closer to commitable state for > 8-CURRENT, but further feedback would be most welcome (including reports of > success on non-i386 architectures, and possibly patches to fix them). For > FreeBSD developers with P4 access, you can also check out Thank you for this. I think procstat(1) is going to be very useful. -- WXS ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
Robert Watson wrote: > I've updated the procstat(1) kernel patch and userland tool; the updated > version can be found at: > > http://www.watson.org/~robert/freebsd/20071127-procstat.tgz > > The new version includes a number of changes from the old version, > including: > > - "-a" now means "all processes", Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a per-process tool. I personally would change it to allow either the all flag or a list of pids, rather than "at least one of". For pathname, command-line, and credential information, the output will likely not change between showing the pid in the "all" output and the "list" output so you're just outputting it twice. If one really wants the same pid to be output multiple times for threads, kstack, or file descriptors, then I'd expect "procstat -k 0 0 0 0 0" to be more useful for that. I would think a mistake in usage has been made if a list of pids is specified along with the "all" flag. But, no real harm is done by doing it the current way. > - Threads and processes are now sorted by pid and then tid. If processes > are specified manually by pid, they are not sorted, although their threads > will be. Nice. > - A new "-k" has been added, which prints the kernel thread stacks for threads > in a process (although not swapped out or actively running threads). This > is extremely useful for answering questions of the sort "But *why* is the > process blocked in UMA". It has both a simple mode (-k_, which lists just > kernel function names, and a slightly more detailed mode (-kk), which adds > the offset into the function. This is excellent. Does this absolutely have to depend on DDB and KDB? > The last of these required new kernel changes, including an MD component. > I've tested the MD parts only on i386, although I have quick hacks at what > they should look like on amd64, arm, powerpc, sparc64, sun4v. I don't > promise these compile or work, but they might do. In sys/amd64/amd64/db_trace.c on line 537, change "SWWAPPED" to "SWAPPED". The newly introducted function stack_save_td() doesn't panic in the MD powerpc code like it does for other arches. I have no idea if this is correct, it just doesn't match the others. Unfortunately, I can only test i386. -- Skip ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
On Tue, 27 Nov 2007, Wesley Shields wrote: Here's an updated patch to sys/amd64/amd64/db_trace.c (it's a diff against revision 1.81). It changes "register rbp" to be "register_t rbp" and fixes the extra "W" in TD_IS_SWAPPED. The kernel built fine after these changes. I'll test it out tomorrow. I've gone ahead and applied that change in Perforce, and look forward to hearing back on the testing. I think procstat(1) is getting a lot closer to commitable state for 8-CURRENT, but further feedback would be most welcome (including reports of success on non-i386 architectures, and possibly patches to fix them). For FreeBSD developers with P4 access, you can also check out Thank you for this. I think procstat(1) is going to be very useful. If you can think of other process-inspection related things it could be doing, let me know. The one thing I currently have in mind that I haven't made progress on is dumping the kernel signal state for the process (i.e., what signals have handlers, etc), which may be useful when debugging signal problems for an application. Robert N M Watson Computer Laboratory University of Cambridge ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
On Wed, 28 Nov 2007, Skip Ford wrote: - "-a" now means "all processes", Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a per-process tool. I was, but then I read your e-mail and became convinced that the first patch that would be submitted against procstat(1) would be a "-a" patch. :-) I personally would change it to allow either the all flag or a list of pids, rather than "at least one of". For pathname, command-line, and credential information, the output will likely not change between showing the pid in the "all" output and the "list" output so you're just outputting it twice. If one really wants the same pid to be output multiple times for threads, kstack, or file descriptors, then I'd expect "procstat -k 0 0 0 0 0" to be more useful for that. I would think a mistake in usage has been made if a list of pids is specified along with the "all" flag. But, no real harm is done by doing it the current way. I think your argument is convincing, and have changed it so that only one of -a and a pidlist can be specified. I've also tightened down the syntax checking on flags a bit more. - A new "-k" has been added, which prints the kernel thread stacks for threads in a process (although not swapped out or actively running threads). This is extremely useful for answering questions of the sort "But *why* is the process blocked in UMA". It has both a simple mode (-k_, which lists just kernel function names, and a slightly more detailed mode (-kk), which adds the offset into the function. This is excellent. Does this absolutely have to depend on DDB and KDB? Currently, yes, as stack(9) is conditional on DDB, and the MD bits of stack(9) are defined in db_trace.c (and in some cases, depend on DDB definitions, such as DDB types, although I think not critically so). I've also been pondering breaking out stack(9) from DDB but haven't done that yet. Maybe that will be today's task, as I'd like -k to work without the kernel debugger, as it has use significantly beyond kernel debugging. In sys/amd64/amd64/db_trace.c on line 537, change "SWWAPPED" to "SWAPPED". Fixed, thanks. The newly introducted function stack_save_td() doesn't panic in the MD powerpc code like it does for other arches. I have no idea if this is correct, it just doesn't match the others. Indeed, and I've now fixed this, thanks! Robert N M Watson Computer Laboratory University of Cambridge ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
Robert Watson wrote: > On Wed, 28 Nov 2007, Skip Ford wrote: > >>>- "-a" now means "all processes", >> >>Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a >>per-process tool. > > I was, but then I read your e-mail and became convinced that the first > patch that would be submitted against procstat(1) would be a "-a" patch. :-) Yep, would've happened. Now the first patch submitted will be a "-w interval" patch... :-) >>>- A new "-k" has been added, which prints the kernel thread stacks for >>>threads >>> in a process (although not swapped out or actively running threads). >>> This >>> is extremely useful for answering questions of the sort "But *why* is >>> the >>> process blocked in UMA". It has both a simple mode (-k_, which lists >>> just >>> kernel function names, and a slightly more detailed mode (-kk), which >>> adds >>> the offset into the function. >> >>This is excellent. Does this absolutely have to depend on DDB and KDB? > > Currently, yes, as stack(9) is conditional on DDB, and the MD bits of > stack(9) are defined in db_trace.c (and in some cases, depend on DDB > definitions, such as DDB types, although I think not critically so). I've > also been pondering breaking out stack(9) from DDB but haven't done that > yet. Maybe that will be today's task, as I'd like -k to work without the > kernel debugger, as it has use significantly beyond kernel debugging. That'd be great if it worked without DDB. It just "feels" like it should. This tool is a very nice addition. Thanks for writing it and for asking for feedback, then putting up with the responses. -- Skip ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
Skip Ford wrote: > Robert Watson wrote: >> On Wed, 28 Nov 2007, Skip Ford wrote: >> - "-a" now means "all processes", >>> >>>Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a >>>per-process tool. >> >> I was, but then I read your e-mail and became convinced that the first >> patch that would be submitted against procstat(1) would be a "-a" patch. :-) > > Yep, would've happened. Now the first patch submitted will be a > "-w interval" patch... :-) I couldn't resist implementing a crude interval arg just for kicks. Here's the output of find(1) every second. This is so cool: $ procstat -k -w 1 948 PIDTID COMM KSTACK 948 100099 find mi_switch thread_suspend_check userret syscall Xint0x80_syscall 948 100099 find mi_switch sleepq_switch sleepq_wait _sleep bwait bufwait breadn bread ffs_read VOP_READ_APV ufs_readdir VOP_READDIR_APV getdirentries syscall Xint0x80_syscall 948 100099 find mi_switch turnstile_wait _mtx_lock_sleep fdalloc falloc kern_open open syscall Xint0x80_syscall 948 100099 find mi_switch sleepq_switch sleepq_wait _sleep bwait bufwait breadn bread ffs_read VOP_READ_APV ufs_readdir VOP_READDIR_APV getdirentries syscall Xint0x80_syscall 948 100099 find mi_switch critical_exit intr_execute_handlers atpic_handle_intr Xatpic_intr0 948 100099 find mi_switch ast doreti_ast 948 100099 find mi_switch sleepq_switch sleepq_wait _sleep bwait bufwait breadn bread ffs_read VOP_READ_APV ufs_readdir VOP_READDIR_APV getdirentries syscall Xint0x80_syscall 948 100099 find mi_switch turnstile_wait _mtx_lock_sleep fdalloc falloc kern_open open syscall Xint0x80_syscall 948 100099 find mi_switch critical_exit intr_execute_handlers atpic_handle_intr Xatpic_intr0 priv_check vn_stat vn_statfile kern_fstat fstat syscall Xint0x80_syscall 948 100099 find mi_switch sleepq_switch sleepq_wait _sleep bwait bufwait breadn bread ffs_vget ufs_lookup VOP_CACHEDLOOKUP_APV vfs_cache_lookup VOP_LOOKUP_APV lookup namei kern_lstat lstat syscall 948 100099 find mi_switch sleepq_switch sleepq_wait _sleep bwait bufwait breadn bread ffs_read VOP_READ_APV ufs_readdir VOP_READDIR_APV getdirentries syscall Xint0x80_syscall For someone who isn't intimately familiar with the kernel, this is really a nice feature for this tool. I'm attaching the very crude interval patch just because it seems rude not to, but you probably either don't want it or will do it differently and either is perfectly fine. If you do implement it on your own, don't bother printing a header per screenful since it would be most useful for kstacks and that output usually spills over to multiple lines anyway. -- Skip diff -u src~/usr.bin/procstat/procstat.c src/usr.bin/procstat/procstat.c --- src~/usr.bin/procstat/procstat.c 2007-11-27 10:23:39.0 -0500 +++ src/usr.bin/procstat/procstat.c 2007-11-28 07:01:23.0 -0500 @@ -106,13 +106,14 @@ main(int argc, char *argv[]) { struct kinfo_proc *kipp; - int ch, i, name[4], tmp; + int ch, i, name[4], tmp, interval; size_t len; long l; pid_t pid; char *dummy; - while ((ch = getopt(argc, argv, "abcfkhstv")) != -1) { + interval = 0; + while ((ch = getopt(argc, argv, "abcfkhstvw:")) != -1) { switch (ch) { case 'a': aflag = 1; @@ -150,6 +151,10 @@ vflag = 1; break; + case 'w': + interval = atoi(optarg); + break; + case '?': default: usage(); @@ -166,6 +171,7 @@ /* Must specify at least one of -a and a list of pids. */ if (!aflag && argc < 1) usage(); +loop: if (aflag) { name[0] = CTL_KERN; name[1] = KERN_PROC; @@ -233,5 +239,11 @@ /* Suppress header after first process. */ hflag = 1; } + + if (interval) { + sleep(interval); + goto loop; + } + exit(0); } ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: Updated procstat(1)
On Wed, 28 Nov 2007, Skip Ford wrote: Skip Ford wrote: Robert Watson wrote: On Wed, 28 Nov 2007, Skip Ford wrote: - "-a" now means "all processes", Thanks. :-) I'm a little surprised. You seemed pretty dedicated to a per-process tool. I was, but then I read your e-mail and became convinced that the first patch that would be submitted against procstat(1) would be a "-a" patch. :-) Yep, would've happened. Now the first patch submitted will be a "-w interval" patch... :-) I couldn't resist implementing a crude interval arg just for kicks. Here's the output of find(1) every second. This is so cool: Very neat :-). If you like this, you'll love DTrace, which allows you to do all sorts of things along these lines. I'll add a -w mode, but be aware that if you want to do the below, what you really want is DTrace :-), which allows you do do things like sample kernel stack traces on the clock timer, based on function invocations, etc, so you can do things like say "sample all the paths to a particular kernel function". Now that John is updating DTrace again, I hope that we'll be seeing it in the 8-CURRENT source RSN. Robert N M Watson Computer Laboratory University of Cambridge ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"