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

[Daniel Thompson <daniel.thompson@linaro.org>]

On Mon, Nov 01, 2021 at 12:56:13PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 1, 2021 at 10:44 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > 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?

/me slaps head

Sorry. This patch really doesn't seem to have been my finest hour.
Really thought I'd got it right at v3 (even threw in extra manual
testing for good measure).

Will fix... immediately.


> > @@ -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"?

Ack.


>  @@ -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)",

Turns out we don't have enough characters to document A on an 80
character terminal (some of the other help text is too long as well but
I'll put that on the list for later).

Will settle for "Backtrace all processes whose state matches" and leave
it at that.

Will also updates the .usage for ps which doesn't use the <state_chars>.


Daniel.