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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20211102161934.664jrypn5rxaszae@maple.lan>
Date:   Tue, 2 Nov 2021 16:19:34 +0000
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        Xiang wangx <wangxiang@...rlc.com>,
        jing yangyang <jing.yangyang@....com.cn>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>,
        Patch Tracking <patches@...aro.org>
Subject: Re: [PATCH v3] kdb: Adopt scheduler's task classification

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@...aro.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.