All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Jason Wessel <jason.wessel@windriver.com>,
	kgdb-bugreport@lists.sourceforge.net,
	Christophe Leroy <christophe.leroy@c-s.fr>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn't round up
Date: Thu, 10 Oct 2019 16:07:35 +0100	[thread overview]
Message-ID: <20191010150735.dhrj3pbjgmjrdpwr@holly.lan> (raw)
In-Reply-To: <CAD=FV=Vqj9JqGCQX_Foij8EkFtSy8r2wB3uoXNae6PECwNV+CQ@mail.gmail.com>

On Mon, Oct 07, 2019 at 04:34:55PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 7, 2019 at 6:55 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Wed, Sep 25, 2019 at 01:02:19PM -0700, Douglas Anderson wrote:
> > >
> > > I noticed that when I did "btc <cpu>" and the CPU I passed in hadn't
> > > rounded up that I'd crash.  I was going to copy the same fix from
> > > commit 162bc7f5afd7 ("kdb: Don't back trace on a cpu that didn't round
> > > up") into the "not all the CPUs" case, but decided it'd be better to
> > > clean things up a little bit.
> > >
> > > This consolidates the two code paths.  It is _slightly_ wasteful in in
> > > that the checks for "cpu" being too small or being offline isn't
> > > really needed when we're iterating over all online CPUs, but that
> > > really shouldn't hurt.  Better to have the same code path.
> > >
> > > While at it, eliminate at least one slightly ugly (and totally
> > > needless) recursive use of kdb_parse().
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > > Changes in v3:
> > > - Patch ("kdb: Fix "btc <cpu>" crash if the CPU...") new for v3.
> > >
> > > Changes in v2: None
> > >
> > >  kernel/debug/kdb/kdb_bt.c | 61 ++++++++++++++++++++++-----------------
> > >  1 file changed, 34 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
> > > index 120fc686c919..d9af139f9a31 100644
> > > --- a/kernel/debug/kdb/kdb_bt.c
> > > +++ b/kernel/debug/kdb/kdb_bt.c
> > > @@ -101,6 +101,27 @@ kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
> > >       return 0;
> > >  }
> > >
> > > +static void
> > > +kdb_bt_cpu(unsigned long cpu)
> > > +{
> > > +     struct task_struct *kdb_tsk;
> > > +
> > > +     if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> > > +             kdb_printf("WARNING: no process for cpu %ld\n", cpu);
> > > +             return;
> > > +     }
> > > +
> > > +     /* If a CPU failed to round up we could be here */
> > > +     kdb_tsk = KDB_TSK(cpu);
> > > +     if (!kdb_tsk) {
> > > +             kdb_printf("WARNING: no task for cpu %ld\n", cpu);
> > > +             return;
> > > +     }
> > > +
> > > +     kdb_set_current_task(kdb_tsk);
> > > +     kdb_bt1(kdb_tsk, ~0UL, false);
> > > +}
> > > +
> > >  int
> > >  kdb_bt(int argc, const char **argv)
> > >  {
> > > @@ -161,7 +182,6 @@ kdb_bt(int argc, const char **argv)
> > >       } else if (strcmp(argv[0], "btc") == 0) {
> > >               unsigned long cpu = ~0;
> > >               struct task_struct *save_current_task = kdb_current_task;
> > > -             char buf[80];
> > >               if (argc > 1)
> > >                       return KDB_ARGCOUNT;
> > >               if (argc == 1) {
> > > @@ -169,35 +189,22 @@ kdb_bt(int argc, const char **argv)
> > >                       if (diag)
> > >                               return diag;
> > >               }
> > > -             /* Recursive use of kdb_parse, do not use argv after
> > > -              * this point */
> > > -             argv = NULL;
> > >               if (cpu != ~0) {
> > > -                     if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> > > -                             kdb_printf("no process for cpu %ld\n", cpu);
> > > -                             return 0;
> > > -                     }
> > > -                     sprintf(buf, "btt 0x%px\n", KDB_TSK(cpu));
> > > -                     kdb_parse(buf);
> > > -                     return 0;
> > > -             }
> > > -             kdb_printf("btc: cpu status: ");
> > > -             kdb_parse("cpu\n");
> > > -             for_each_online_cpu(cpu) {
> > > -                     void *kdb_tsk = KDB_TSK(cpu);
> > > -
> > > -                     /* If a CPU failed to round up we could be here */
> > > -                     if (!kdb_tsk) {
> > > -                             kdb_printf("WARNING: no task for cpu %ld\n",
> > > -                                        cpu);
> > > -                             continue;
> > > +                     kdb_bt_cpu(cpu);
> > > +             } else {
> > > +                     /*
> > > +                      * Recursive use of kdb_parse, do not use argv after
> > > +                      * this point.
> > > +                      */
> > > +                     argv = NULL;
> > > +                     kdb_printf("btc: cpu status: ");
> > > +                     kdb_parse("cpu\n");
> > > +                     for_each_online_cpu(cpu) {
> > > +                             kdb_bt_cpu(cpu);
> > > +                             touch_nmi_watchdog();
> > >                       }
> > > -
> > > -                     sprintf(buf, "btt 0x%px\n", kdb_tsk);
> > > -                     kdb_parse(buf);
> > > -                     touch_nmi_watchdog();
> > > +                     kdb_set_current_task(save_current_task);
> > >               }
> > > -             kdb_set_current_task(save_current_task);
> >
> > Why does this move out into only one of the conditional branches?
> > Don't both of the above paths modify the current task?
> 
> The old code has a "return 0 in the case that "cpu != ~0", so this
> basically matches the prior behavior in restoring the current task for
> a "btc" but not leaving the current task changed in the case of "btc
> <cpu>".  Thus my patch doesn't actually change the existing behavior,
> but I guess that it does make the control flow simpler so it's easier
> to understand what the behavior is.  ;-)

Point taken. Horrific though it may be ;-) .


> Reading through other control flows of the various backtrace commands,
> it looks like it is intentional to leave the current task changed when
> you explicitly do an action on that task (or a CPU).
> 
> Actually, though, it wasn't clear to me that it ever made sense for
> any of these commands to implicitly leave the current task changed.
> If you agree, I can send a follow-up patch to change this behavior.

Personally I don't like implicit changes of state but I might need a bit
more thinking to agree (or disagree ;-) ).


Daniel.

  reply	other threads:[~2019-10-10 15:07 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 20:02 [PATCH v3 0/4] kdb: Fixes for btc Douglas Anderson
2019-09-25 20:02 ` [PATCH v3 1/4] kgdb: Remove unused DCPU_SSTEP definition Douglas Anderson
2019-09-25 20:08   ` Jason Wessel
2019-09-25 20:02 ` [PATCH v3 2/4] kdb: Remove unused "argcount" param from kdb_bt1(); make btaprompt bool Douglas Anderson
2019-09-25 20:02 ` [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn't round up Douglas Anderson
2019-10-03 21:09   ` Will Deacon
2019-10-07 13:54   ` Daniel Thompson
2019-10-07 23:34     ` Doug Anderson
2019-10-10 15:07       ` Daniel Thompson [this message]
2019-10-10 16:38         ` Doug Anderson
2019-11-09 19:20           ` Doug Anderson
2019-09-25 20:02 ` [PATCH v3 4/4] kdb: Fix stack crawling on 'running' CPUs that aren't the master Douglas Anderson
2019-10-03 21:11 ` [PATCH v3 0/4] kdb: Fixes for btc Will Deacon
2019-10-10 16:51 ` Daniel Thompson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191010150735.dhrj3pbjgmjrdpwr@holly.lan \
    --to=daniel.thompson@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=christophe.leroy@c-s.fr \
    --cc=dianders@chromium.org \
    --cc=jason.wessel@windriver.com \
    --cc=kgdb-bugreport@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn'\''t round up' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.