Page MenuHomeFreeBSD

kern.tty_info_kstacks: add a compact format
ClosedPublic

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/kern/subr_stack.c
185 ↗(On Diff #73759)

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.

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

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.

love it

sys/sys/stack.h
34 ↗(On Diff #73759)

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

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

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).

sys/kern/tty_info.c
255 ↗(On Diff #73759)

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

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 added inline comments.
sys/kern/subr_stack.c
185 ↗(On Diff #73759)

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
255 ↗(On Diff #73759)

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

242 ↗(On Diff #73777)

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

sys/sys/stack.h
34 ↗(On Diff #73759)

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

kaktus added inline comments.
sys/kern/tty_info.c
255 ↗(On Diff #73759)

s/current/correct/

sys/kern/subr_stack.c
169–174 ↗(On Diff #73777)

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

186–188 ↗(On Diff #73777)

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 ↗(On Diff #73777)

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

sys/kern/tty_info.c
255 ↗(On Diff #73759)

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.

242 ↗(On Diff #73777)

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

366 ↗(On Diff #73777)

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.

sys/kern/tty_info.c
366 ↗(On Diff #73777)

Good catch, will fix.

sys/kern/subr_stack.c
169–174 ↗(On Diff #73777)

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

Use correct diff this time…

sys/kern/tty_info.c
367 ↗(On Diff #74066)

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

This revision is now accepted and ready to land.Jul 4 2020, 11:15 AM
cem requested changes to this revision.Jul 4 2020, 6:05 PM
cem added inline comments.
sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

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

169–174 ↗(On Diff #73777)

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

sys/kern/tty_info.c
242 ↗(On Diff #74067)

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

368–371 ↗(On Diff #74067)

print_kstacks = (kstacks_val != STACK_SBUF_FMT_NONE); ?

This revision now requires changes to proceed.Jul 4 2020, 6:05 PM
sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

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

sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

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

sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

you can __assert_unreachable in the default case

sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

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

sys/kern/subr_stack.c
194–195 ↗(On Diff #74067)

There is also __assert_unreachable() now.

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

__assert_unreachable in correct place

This revision was not accepted when it landed; it landed in state Needs Review.Jul 6 2020, 4:33 PM
This revision was automatically updated to reflect the committed changes.