Re: [PATCH v3] kdb: Adopt scheduler's task classification

[Doug Anderson <dianders@chromium.org>]

Hi,

On Mon, Nov 1, 2021 at 10:44 AM Daniel Thompson
<daniel.thompson@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.thompson@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