Page MenuHomeFreeBSD

kern.tty_info_kstacks: add a compact format
Needs ReviewPublic

Authored by kaktus on Sat, Jun 27, 2:00 PM.

Details

Reviewers
avg
jhb
kib
mjg
cem
Summary

Add a more compact display format for kern.tty_info_kstacks inspired by procstat -kk. Set it as a default one.

Suggested by: avg
Relnotes: yes, please

Test Plan
root@devel:~ # sysctl kern.tty_info_kstacks=1
root@devel:~ # sleep 2
^T
load: 0.19  cmd: sleep 619 [nanslp] 0.66r 0.00u 0.00s 0% 2124k
#0 0xffffffff80c4443e at mi_switch+0xbe
#1 0xffffffff80c98044 at sleepq_catch_signals+0x494
#2 0xffffffff80c982c2 at sleepq_timedwait_sig+0x12
#3 0xffffffff80c43af3 at _sleep+0x193
#4 0xffffffff80c50e31 at kern_clock_nanosleep+0x1a1
#5 0xffffffff80c5119b at sys_nanosleep+0x3b
#6 0xffffffff810ffc69 at amd64_syscall+0x119
#7 0xffffffff810d5410 at fast_syscall_common+0x101
sleep: about 1 second(s) left out of the original 2
^C
root@devel:~ # sysctl kern.tty_info_kstacks=2
kern.tty_info_kstacks: 1 -> 2
root@devel:~ # sleep 2
^T
load: 0.16  cmd: sleep 622 [nanslp] 0.89r 0.00u 0.00s 0% 2124k
mi_switch+0xbe sleepq_catch_signals+0x494 sleepq_timedwait_sig+0x12 _sleep+0x193 kern_clock_nanosleep+0x1a1 sys_nanosleep+0x3b amd64_syscall+0x119 fast_syscall_common+0x101
sleep: about 1 second(s) left out of the original 2

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kaktus created this revision.Sat, Jun 27, 2:00 PM
kaktus requested review of this revision.Sat, Jun 27, 2:00 PM
mjg added inline comments.Sat, Jun 27, 2:09 PM
sys/kern/subr_stack.c
186–196

This routine is effectively used by witness and other stack-printers, meaning the tty-related change alters other output.

Instead you should add an argument specifying the format (can be an enum) -- at least "long" and "short". Then the tty-related caller can pass the right value depending on tty_info_kstacks.

This routine can be renamed to add _format to it and the current name can become a macro which expands to stack_sbuf_print_flags_format(..., STACK_SBUF_LONG) or similar.

mjg added a reviewer: cem.EditedSat, Jun 27, 2:10 PM

Side note is that I think the shorter format should be the default.

avg added a comment.Sat, Jun 27, 2:15 PM

I agree with both @mjg 's suggestions.

And, just in case, I prefer "compact" over "short" if it appears anywhere in names or documents.
But that's just my personal preference, feel free to ignore it.

imp added a comment.Sat, Jun 27, 2:22 PM

love it

sys/sys/stack.h
34

This file could be done separately since it's a bit orthogonal to this change (the extern isn't, but the _KERNEL part is).

cem added a comment.Sat, Jun 27, 3:07 PM

I’m not keen on the short format but I prefer it to “off.”

pstef added a subscriber: pstef.EditedSat, Jun 27, 3:10 PM
In D25487#562808, @mjg wrote:

Side note is that I think the shorter format should be the default.

The shorter format is less readable, but the long format is a jump-scare to anyone not familiar with the new behavior (so probably all non-developer users).

cem added inline comments.Sat, Jun 27, 3:13 PM
sys/kern/tty_info.c
254

Don’t we have a range-limited sysctl mechanism already?

kaktus updated this revision to Diff 73777.Sat, Jun 27, 4:19 PM
kaktus retitled this revision from kern.tty_info_kstacks: add a short format to kern.tty_info_kstacks: add a compact format.
kaktus edited the summary of this revision. (Show Details)

Address comments.

kaktus marked 2 inline comments as done.Sat, Jun 27, 4:23 PM
kaktus added inline comments.
sys/kern/subr_stack.c
186–196

I assume you meant stack_sbuf_print(). I adjusted it to always use long format. I find stack_sbuf_print_flags() a perfect name for a function that takes flags as an arguments though.

sys/kern/tty_info.c
242–271

I believe it should be enough in most cases even for developers?

254

Not that I know of, but if checking for a current value isn't welcomed it can be squashed to bare SYSCTL_INT.

sys/sys/stack.h
34

I found that few other headers are missing that \#ifdef, i'll do it in separate patch.

kaktus marked 2 inline comments as done.Sat, Jun 27, 4:50 PM
kaktus added inline comments.
sys/kern/tty_info.c
254

s/current/correct/

cem added inline comments.Sat, Jun 27, 4:59 PM
sys/kern/subr_stack.c
169–175

I'd encourage adding this to the existing flags parameter. Only need one bit for FMT_COMPACT.

186–198

Calling stack_sbuf_print with FMT_NONE is nonsensical. If anything, throw a KASSERT(format != FMT_NONE) at top of function. It does not make sense to handle it in every loop iteration.

196–197

Kernel consumers should have correct API usage. Just assert API correctness.

sys/kern/tty_info.c
242–271

They print similar information, but I find the one-per-line format much more legible.

254

dev/random/randomdev.h RANDOM_CHECK_UINT is probably what I was thinking of. I don't think it's quite what we'd want here (we just an upper limit encoded in arg2). I think it might be nice to eventually have a range-limited sysctl mechanism, but of course it is orthogonal to this change.

367–370

Probably want to load the value of tty_info_kstacks once, as the sysctl could change it between here and the use below, causing use-of-uninitialized-stack-values.

kaktus added inline comments.Sat, Jun 27, 5:20 PM
sys/kern/tty_info.c
367–370

Good catch, will fix.

kaktus updated this revision to Diff 74065.Sat, Jul 4, 10:35 AM

Address /some/ comments.

kaktus added inline comments.Sat, Jul 4, 10:37 AM
sys/kern/subr_stack.c
169–175

Until one day someone decide to use that one bit as a malloc(9) flag that is passed in flags :-(

kaktus updated this revision to Diff 74066.Sat, Jul 4, 10:39 AM

Use correct diff this time…

mjg added inline comments.Sat, Jul 4, 10:50 AM
sys/kern/tty_info.c
367–370

for safety this should be atomic_load_int(&tty_info_kstacks) to make sure the compiler never reloads this, looks good otherwise

kaktus updated this revision to Diff 74067.Sat, Jul 4, 11:05 AM

Use atomic_load

mjg accepted this revision.Sat, Jul 4, 11:15 AM
This revision is now accepted and ready to land.Sat, Jul 4, 11:15 AM
cem requested changes to this revision.Sat, Jul 4, 6:05 PM
cem added inline comments.
sys/kern/subr_stack.c
169–175

You can _Static_assert that it stays true if you are concerned.

194–195

This is still wrong -- kernel consumers should pass a correct value.

sys/kern/tty_info.c
242–271

Should the additional functionality be separate from the change in default?

368–371

print_kstacks = (kstacks_val != STACK_SBUF_FMT_NONE); ?

This revision now requires changes to proceed.Sat, Jul 4, 6:05 PM
kaktus added inline comments.Sat, Jul 4, 6:48 PM
sys/kern/subr_stack.c
194–195

This is enum, -Werror,-Wswitch (correctly) doesn't like when I don't check all variables.

cem added inline comments.Sat, Jul 4, 6:54 PM
sys/kern/subr_stack.c
194–195

So perhaps switch is an inappropriate construct here when if/else would be more accurate, in addition to thwarting false-positive compiler warnings?

mjg added inline comments.Sat, Jul 4, 6:55 PM
sys/kern/subr_stack.c
194–195

you can __assert_unreachable in the default case

kaktus added inline comments.Sat, Jul 4, 6:56 PM
sys/kern/subr_stack.c
194–195

This isn't the only place in kernel that does that.

cem added inline comments.Sat, Jul 4, 6:57 PM
sys/kern/subr_stack.c
194–195

There is also __assert_unreachable() now.

kaktus updated this revision to Diff 74079.Sat, Jul 4, 7:11 PM

Use __assert_unreachable().
Shorten the code by two LOC.

kaktus updated this revision to Diff 74082.Sat, Jul 4, 7:26 PM

__assert_unreachable in correct place

mjg accepted this revision.Sun, Jul 5, 6:13 AM