Page MenuHomeFreeBSD

kern.tty_info_kstacks: add a compact format
ClosedPublic

Authored by kaktus on Jun 27 2020, 2:00 PM.
Tags
None
Referenced Files
F80133482: D25487.id74065.diff
Thu, Mar 28, 8:57 AM
Unknown Object (File)
Mon, Mar 18, 7:53 PM
Unknown Object (File)
Mon, Mar 18, 5:44 PM
Unknown Object (File)
Feb 13 2024, 11:37 AM
Unknown Object (File)
Feb 10 2024, 1:39 AM
Unknown Object (File)
Jan 25 2024, 12:15 AM
Unknown Object (File)
Jan 17 2024, 7:40 PM
Unknown Object (File)
Jan 8 2024, 6:51 PM
Subscribers

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.

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

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
254

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
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 added inline comments.
sys/kern/tty_info.c
254

s/current/correct/

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.

sys/kern/tty_info.c
367–370

Good catch, will fix.

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 :-(

Use correct diff this time…

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

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
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.Jul 4 2020, 6:05 PM
sys/kern/subr_stack.c
194–195

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

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?

sys/kern/subr_stack.c
194–195

you can __assert_unreachable in the default case

sys/kern/subr_stack.c
194–195

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

sys/kern/subr_stack.c
194–195

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.