Page MenuHomeFreeBSD
Differential D25487

kern.tty_info_kstacks: add a compact format
ClosedPublic
Actions

Authored by kaktus on Jun 27 2020, 2:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 19, 8:26 AM
Unknown Object (File)
Mon, Apr 14, 5:08 AM
Unknown Object (File)
Sun, Apr 13, 10:05 AM
Unknown Object (File)
Sat, Mar 29, 6:14 PM
Unknown Object (File)
Mon, Mar 24, 1:25 AM
Unknown Object (File)
Mar 19 2025, 4:27 AM
Unknown Object (File)
Mar 16 2025, 9:48 AM
Unknown Object (File)
Mar 8 2025, 7:30 AM
View All Files
Subscribers
imp
pstef

Details

Reviewers
avg
jhb
kib
mjg
cem
Commits
rS362967: kern.tty_info_kstacks: add a compact format
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

kaktus created this revision.Jun 27 2020, 2:00 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 27 2020, 2:00 PM
kaktus requested review of this revision.Jun 27 2020, 2:00 PM
Harbormaster completed remote builds in B32010: Diff 73759.Jun 27 2020, 2:01 PM
mjg added inline comments.Jun 27 2020, 2:09 PM
sys/kern/subr_stack.c
186–198

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.EditedJun 27 2020, 2:10 PM

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

avg added a comment.Jun 27 2020, 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.Jun 27 2020, 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.Jun 27 2020, 3:07 PM

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

pstef added a subscriber: pstef.EditedJun 27 2020, 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.Jun 27 2020, 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.Jun 27 2020, 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.Jun 27 2020, 4:23 PM
kaktus added inline comments.
sys/kern/subr_stack.c
186–198

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.Jun 27 2020, 4:50 PM
kaktus added inline comments.
sys/kern/tty_info.c
254

s/current/correct/

cem added inline comments.Jun 27 2020, 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–200

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.

366

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.Jun 27 2020, 5:20 PM
sys/kern/tty_info.c
366

Good catch, will fix.

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

Address /some/ comments.

kaktus added inline comments.Jul 4 2020, 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.Jul 4 2020, 10:39 AM

Use correct diff this time…

mjg added inline comments.Jul 4 2020, 10:50 AM
sys/kern/tty_info.c
366

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.Jul 4 2020, 11:05 AM

Use atomic_load

mjg accepted this revision.Jul 4 2020, 11:15 AM
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?

367–370

print_kstacks = (kstacks_val != STACK_SBUF_FMT_NONE); ?

This revision now requires changes to proceed.Jul 4 2020, 6:05 PM
kaktus added inline comments.Jul 4 2020, 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.Jul 4 2020, 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.Jul 4 2020, 6:55 PM
sys/kern/subr_stack.c
194–195

you can __assert_unreachable in the default case

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

There is also __assert_unreachable() now.

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

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

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

__assert_unreachable in correct place

mjg accepted this revision.Jul 5 2020, 6:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 6 2020, 4:33 PM
Closed by commit rS362967: kern.tty_info_kstacks: add a compact format (authored by kaktus). · Explain Why
This revision was automatically updated to reflect the committed changes.
kaktus added a commit: rS362967: kern.tty_info_kstacks: add a compact format.

Revision Contents
Changeset List

PathSize
sys/
kern/
21 lines
40 lines
sys/
8 lines

Diff 74065

View Options

sys/kern/subr_stack.c

Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Lines
/* /*
* Resource usage can drop, as opposed to only grow. * Resource usage can drop, as opposed to only grow.
*/ */
#define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X)) #define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X))
/* /*
* The 'racct' structure defines resource consumption for a particular * The 'racct' structure defines resource consumption for a particular
* subject, such as process or jail. * subject, such as process or jail. It also contains the total
* cpu time and real time of the subject, recorded at the most recent
* time that RACCT_PCPU was updated.
markjUnsubmitted
Not Done
Inline Actions

s/current/total/, I think. Or perhaps "accumulated".

markj: s/current/total/, I think. Or perhaps "accumulated".
* *
* This structure must be filled with zeroes initially. * This structure must be filled with zeroes initially.
*/ */
struct racct { struct racct {
int64_t r_resources[RACCT_MAX + 1]; int64_t r_resources[RACCT_MAX + 1];
LIST_HEAD(, rctl_rule_link) r_rule_links; LIST_HEAD(, rctl_rule_link) r_rule_links;
uint64_t r_runtime;
struct timeval r_time;
markjUnsubmitted
Not Done
Inline Actions

It would be worthwhile to add a few sentences to the comment above explaining these fields.

markj: It would be worthwhile to add a few sentences to the comment above explaining these fields.
}; };
SYSCTL_DECL(_kern_racct); SYSCTL_DECL(_kern_racct);
#ifdef RACCT #ifdef RACCT
extern struct mtx racct_lock; extern struct mtx racct_lock;
▲ Show 20 Lines • Show All 121 Lines • Show Last 20 Lines
View Options

sys/kern/tty_info.c

Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Lines
/* /*
* Resource usage can drop, as opposed to only grow. * Resource usage can drop, as opposed to only grow.
*/ */
#define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X)) #define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X))
/* /*
* The 'racct' structure defines resource consumption for a particular * The 'racct' structure defines resource consumption for a particular
* subject, such as process or jail. * subject, such as process or jail. It also contains the total
* cpu time and real time of the subject, recorded at the most recent
* time that RACCT_PCPU was updated.
markjUnsubmitted
Not Done
Inline Actions

s/current/total/, I think. Or perhaps "accumulated".

markj: s/current/total/, I think. Or perhaps "accumulated".
* *
* This structure must be filled with zeroes initially. * This structure must be filled with zeroes initially.
*/ */
struct racct { struct racct {
int64_t r_resources[RACCT_MAX + 1]; int64_t r_resources[RACCT_MAX + 1];
LIST_HEAD(, rctl_rule_link) r_rule_links; LIST_HEAD(, rctl_rule_link) r_rule_links;
uint64_t r_runtime;
struct timeval r_time;
markjUnsubmitted
Not Done
Inline Actions

It would be worthwhile to add a few sentences to the comment above explaining these fields.

markj: It would be worthwhile to add a few sentences to the comment above explaining these fields.
}; };
SYSCTL_DECL(_kern_racct); SYSCTL_DECL(_kern_racct);
#ifdef RACCT #ifdef RACCT
extern struct mtx racct_lock; extern struct mtx racct_lock;
▲ Show 20 Lines • Show All 121 Lines • Show Last 20 Lines
View Options

sys/sys/stack.h

Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Lines
/* /*
* Resource usage can drop, as opposed to only grow. * Resource usage can drop, as opposed to only grow.
*/ */
#define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X)) #define RACCT_CAN_DROP(X) (RACCT_IS_RECLAIMABLE(X) | RACCT_IS_DECAYING(X))
/* /*
* The 'racct' structure defines resource consumption for a particular * The 'racct' structure defines resource consumption for a particular
* subject, such as process or jail. * subject, such as process or jail. It also contains the total
* cpu time and real time of the subject, recorded at the most recent
* time that RACCT_PCPU was updated.
markjUnsubmitted
Not Done
Inline Actions

s/current/total/, I think. Or perhaps "accumulated".

markj: s/current/total/, I think. Or perhaps "accumulated".
* *
* This structure must be filled with zeroes initially. * This structure must be filled with zeroes initially.
*/ */
struct racct { struct racct {
int64_t r_resources[RACCT_MAX + 1]; int64_t r_resources[RACCT_MAX + 1];
LIST_HEAD(, rctl_rule_link) r_rule_links; LIST_HEAD(, rctl_rule_link) r_rule_links;
uint64_t r_runtime;
struct timeval r_time;
markjUnsubmitted
Not Done
Inline Actions

It would be worthwhile to add a few sentences to the comment above explaining these fields.

markj: It would be worthwhile to add a few sentences to the comment above explaining these fields.
}; };
SYSCTL_DECL(_kern_racct); SYSCTL_DECL(_kern_racct);
#ifdef RACCT #ifdef RACCT
extern struct mtx racct_lock; extern struct mtx racct_lock;
▲ Show 20 Lines • Show All 121 Lines • Show Last 20 Lines