Hi,
On Mon, Nov 1, 2021 at 10:44 AM Daniel Thompson
<daniel.thomp...@linaro.org> wrote:
>
> Currently kdb contains some open-coded routines to generate a summary
> character for each task. This code currently issues warnings, is
> almost certainly broken and won't make sense to any kernel dev who
> has ever used /proc to examine task states.
>
> Fix both the warning and the potential for confusion by adopting the
> scheduler's task classification. Whilst doing this we also simplify the
> filtering by using mask strings directly (which means we don't have to
> guess all the characters the scheduler might give us).
>
> Unfortunately we can't quite match the scheduler classification completely.
> We add four extra states: - for idle loops and i, m and s sleeping system
> daemons (which means kthreads in one of the I, M and S states). These
> extra states are used to manage the filters for tools to make the output
> of ps and bta less noisy.
>
> Note: The Fixes below is the last point the original dubious code was
> moved; it was not introduced by that patch. However it gives us
> the last point to which this patch can be easily backported.
> Happily that should be enough to cover the introduction of
> CONFIG_WERROR!
>
> Fixes: 2f064a59a11f ("sched: Change task_struct::state")
> Signed-off-by: Daniel Thompson <daniel.thomp...@linaro.org>
> ---
>
> Notes:
> v3:
> - Fix the uninitialized cpu variable (Doug and 0-day CI bot)
> - Added a Fixes: (Doug)
> - Changed "state I" -> "state -" and "state M" to "state [ism]"
>
> v2:
> - Fix the typos in the description (Doug)
> - Stop trying to bend to world so I can keep 'I' exactly as
> it was before. Instead we now replace 'I' with '-' and
> fully adopt the scheduler description of tasks. kdb
> it an interactive tool, not ABI so this is OK. (Doug)
> - Don't try to enumerate all possible letters in the
> comments and help. You can learn what to filter from
> the output of ps anyway, (Doug)
> - Fix the sleeping system daemon stuff.
>
> kernel/debug/kdb/kdb_bt.c | 14 ++--
> kernel/debug/kdb/kdb_main.c | 35 +++++-----
> kernel/debug/kdb/kdb_private.h | 4 +-
> kernel/debug/kdb/kdb_support.c | 118 +++++++--------------------------
> 4 files changed, 51 insertions(+), 120 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
> index 1f9f0e47aeda..3368a2d15d73 100644
> --- a/kernel/debug/kdb/kdb_bt.c
> +++ b/kernel/debug/kdb/kdb_bt.c
> @@ -74,7 +74,7 @@ static void kdb_show_stack(struct task_struct *p, void
> *addr)
> */
>
> static int
> -kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
> +kdb_bt1(struct task_struct *p, const char *mask, bool btaprompt)
(Copied from v1 review):
In the comment above this function there is still a reference to
"DRSTCZEUIMA". Update that?
> @@ -2271,37 +2271,30 @@ static int kdb_cpu(int argc, const char **argv)
> void kdb_ps_suppressed(void)
> {
> int idle = 0, daemon = 0;
> - unsigned long mask_I = kdb_task_state_string("I"),
> - mask_M = kdb_task_state_string("M");
> unsigned long cpu;
> const struct task_struct *p, *g;
> for_each_online_cpu(cpu) {
> p = kdb_curr_task(cpu);
> - if (kdb_task_state(p, mask_I))
> + if (kdb_task_state(p, "-"))
> ++idle;
> }
> for_each_process_thread(g, p) {
> - if (kdb_task_state(p, mask_M))
> + if (kdb_task_state(p, "ims"))
> ++daemon;
> }
> if (idle || daemon) {
> if (idle)
> - kdb_printf("%d idle process%s (state I)%s\n",
> + kdb_printf("%d idle process%s (state -)%s\n",
> idle, idle == 1 ? "" : "es",
> daemon ? " and " : "");
> if (daemon)
> - kdb_printf("%d sleeping system daemon (state M) "
> + kdb_printf("%d sleeping system daemon (state [ism]) "
super nitty: elsewhere you use "ims", not "ism". Can you be consistent
and change the above to "ims"?
@@ -2742,8 +2743,8 @@ static kdbtab_t maintab[] = {
> },
> { .name = "bta",
> .func = kdb_bt,
> - .usage = "[D|R|S|T|C|Z|E|U|I|M|A]",
> - .help = "Backtrace all processes matching state flag",
> + .usage = "[<state_chars>|A]",
> + .help = "Backtrace all processes matching whose state
> matches",
"matching whose state matches" sounds odd. Clean it up and use the
saved chars to document "A":
.help = "Backtrace all processes whose state matches (A=all)",
-Doug
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
