by Daniel Thompson

[permalink] [raw]

Subject: Re: [REPOST PATCH v6 3/4] kgdb: Don't round up a CPU that failed rounding up before

On Tue, Dec 04, 2018 at 07:38:27PM -0800, Douglas Anderson wrote:
> If we're using the default implementation of kgdb_roundup_cpus() that
> uses smp_call_function_single_async() we can end up hanging
> kgdb_roundup_cpus() if we try to round up a CPU that failed to round
> up before.
>
> Specifically smp_call_function_single_async() will try to wait on the
> csd lock for the CPU that we're trying to round up. If the previous
> round up never finished then that lock could still be held and we'll
> just sit there hanging.
>
> There's not a lot of use trying to round up a CPU that failed to round
> up before. Let's keep a flag that indicates whether the CPU started
> but didn't finish to round up before. If we see that flag set then
> we'll skip the next round up.
>
> In general we have a few goals here:
> - We never want to end up calling smp_call_function_single_async()
> when the csd is still locked. This is accomplished because
> flush_smp_call_function_queue() unlocks the csd _before_ invoking
> the callback. That means that when kgdb_nmicallback() runs we know
> for sure the the csd is no longer locked. Thus when we set
> "rounding_up = false" we know for sure that the csd is unlocked.
> - If there are no timeouts rounding up we should never skip a round
> up.
>
> NOTE #1: In general trying to continue running after failing to round
> up CPUs doesn't appear to be supported in the debugger. When I
> simulate this I find that kdb reports "Catastrophic error detected"
> when I try to continue. I can overrule and continue anyway, but it
> should be noted that we may be entering the land of dragons here.
> Possibly the "Catastrophic error detected" was added _because_ of the
> future failure to round up, but even so this is an area of the code
> that hasn't been strongly tested.
>
> NOTE #2: I did a bit of testing before and after this change. I
> introduced a 10 second hang in the kernel while holding a spinlock
> that I could invoke on a certain CPU with 'taskset -c 3 cat /sys/...".
>
> Before this change if I did:
> - Invoke hang
> - Enter debugger
> - g (which warns about Catastrophic error, g again to go anyway)
> - g
> - Enter debugger
>
> ...I'd hang the rest of the 10 seconds without getting a debugger
> prompt. After this change I end up in the debugger the 2nd time after
> only 1 second with the standard warning about 'Timed out waiting for
> secondary CPUs.'
>
> I'll also note that once the CPU finished waiting I could actually
> debug it (aka "btc" worked)
>
> I won't promise that everything works perfectly if the errant CPU
> comes back at just the wrong time (like as we're entering or exiting
> the debugger) but it certainly seems like an improvement.
>
> NOTE #3: setting 'kgdb_info[cpu].rounding_up = false' is in
> kgdb_nmicallback() instead of kgdb_call_nmi_hook() because some
> implementations override kgdb_call_nmi_hook(). It shouldn't hurt to
> have it in kgdb_nmicallback() in any case.
>
> NOTE #4: this logic is really only needed because there is no API call
> like "smp_try_call_function_single_async()" or "smp_csd_is_locked()".
> If such an API existed then we'd use it instead, but it seemed a bit
> much to add an API like this just for kgdb.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>

Applied! Thanks.

> ---
>
> Changes in v6:
> - Moved smp_call_function_single_async() error check to patch 3.
>
> Changes in v5: None
> Changes in v4:
> - Removed smp_mb() calls.
>
> Changes in v3:
> - Don't round up a CPU that failed rounding up before new for v3.
>
> Changes in v2: None
>
> kernel/debug/debug_core.c | 20 +++++++++++++++++++-
> kernel/debug/debug_core.h | 1 +
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 10db2833a423..1fb8b239e567 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -247,6 +247,7 @@ void __weak kgdb_roundup_cpus(void)
> call_single_data_t *csd;
> int this_cpu = raw_smp_processor_id();
> int cpu;
> + int ret;
>
> for_each_online_cpu(cpu) {
> /* No need to roundup ourselves */
> @@ -254,8 +255,23 @@ void __weak kgdb_roundup_cpus(void)
> continue;
>
> csd = &per_cpu(kgdb_roundup_csd, cpu);
> +
> + /*
> + * If it didn't round up last time, don't try again
> + * since smp_call_function_single_async() will block.
> + *
> + * If rounding_up is false then we know that the
> + * previous call must have at least started and that
> + * means smp_call_function_single_async() won't block.
> + */
> + if (kgdb_info[cpu].rounding_up)
> + continue;
> + kgdb_info[cpu].rounding_up = true;
> +
> csd->func = kgdb_call_nmi_hook;
> - smp_call_function_single_async(cpu, csd);
> + ret = smp_call_function_single_async(cpu, csd);
> + if (ret)
> + kgdb_info[cpu].rounding_up = false;
> }
> }
>
> @@ -788,6 +804,8 @@ int kgdb_nmicallback(int cpu, void *regs)
> struct kgdb_state kgdb_var;
> struct kgdb_state *ks = &kgdb_var;
>
> + kgdb_info[cpu].rounding_up = false;
> +
> memset(ks, 0, sizeof(struct kgdb_state));
> ks->cpu = cpu;
> ks->linux_regs = regs;
> diff --git a/kernel/debug/debug_core.h b/kernel/debug/debug_core.h
> index 127d9bc49fb4..b4a7c326d546 100644
> --- a/kernel/debug/debug_core.h
> +++ b/kernel/debug/debug_core.h
> @@ -42,6 +42,7 @@ struct debuggerinfo_struct {
> int ret_state;
> int irq_depth;
> int enter_kgdb;
> + bool rounding_up;
> };
>
> extern struct debuggerinfo_struct kgdb_info[];
> --
> 2.20.0.rc1.387.gf8505762e3-goog
>

2018-12-19 21:22:24

by Daniel Thompson

[permalink] [raw]

Subject: Re: [REPOST PATCH v6 4/4] kdb: Don't back trace on a cpu that didn't round up

On Tue, Dec 04, 2018 at 07:38:28PM -0800, Douglas Anderson wrote:
> If you have a CPU that fails to round up and then run 'btc' you'll end
> up crashing in kdb becaue we dereferenced NULL. Let's add a check.
> It's wise to also set the task to NULL when leaving the debugger so
> that if we fail to round up on a later entry into the debugger we
> won't backtrace a stale task.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>

Applied! Thanks.

> ---
>
> Changes in v6: None
> Changes in v5: None
> Changes in v4:
> - Also clear out .debuggerinfo.
> - Also clear out .debuggerinfo and .task for the master.
> - Remove clearing out in kdb_stub for offline CPUs; it's now redundant.
>
> Changes in v3:
> - Don't back trace on a cpu that didn't round up new for v3.
>
> Changes in v2: None
>
> kernel/debug/debug_core.c | 4 ++++
> kernel/debug/kdb/kdb_bt.c | 11 ++++++++++-
> kernel/debug/kdb/kdb_debugger.c | 7 -------
> 3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 1fb8b239e567..5cc608de6883 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -592,6 +592,8 @@ static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
> arch_kgdb_ops.correct_hw_break();
> if (trace_on)
> tracing_on();
> + kgdb_info[cpu].debuggerinfo = NULL;
> + kgdb_info[cpu].task = NULL;
> kgdb_info[cpu].exception_state &=
> ~(DCPU_WANT_MASTER | DCPU_IS_SLAVE);
> kgdb_info[cpu].enter_kgdb--;
> @@ -724,6 +726,8 @@ static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
> if (trace_on)
> tracing_on();
>
> + kgdb_info[cpu].debuggerinfo = NULL;
> + kgdb_info[cpu].task = NULL;
> kgdb_info[cpu].exception_state &=
> ~(DCPU_WANT_MASTER | DCPU_IS_SLAVE);
> kgdb_info[cpu].enter_kgdb--;
> diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
> index 7921ae4fca8d..7e2379aa0a1e 100644
> --- a/kernel/debug/kdb/kdb_bt.c
> +++ b/kernel/debug/kdb/kdb_bt.c
> @@ -186,7 +186,16 @@ kdb_bt(int argc, const char **argv)
> kdb_printf("btc: cpu status: ");
> kdb_parse("cpu\n");
> for_each_online_cpu(cpu) {
> - sprintf(buf, "btt 0x%px\n", KDB_TSK(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;
> + }
> +
> + sprintf(buf, "btt 0x%px\n", kdb_tsk);
> kdb_parse(buf);
> touch_nmi_watchdog();
> }
> diff --git a/kernel/debug/kdb/kdb_debugger.c b/kernel/debug/kdb/kdb_debugger.c
> index 15e1a7af5dd0..53a0df6e4d92 100644
> --- a/kernel/debug/kdb/kdb_debugger.c
> +++ b/kernel/debug/kdb/kdb_debugger.c
> @@ -118,13 +118,6 @@ int kdb_stub(struct kgdb_state *ks)
> kdb_bp_remove();
> KDB_STATE_CLEAR(DOING_SS);
> KDB_STATE_SET(PAGER);
> - /* zero out any offline cpu data */
> - for_each_present_cpu(i) {
> - if (!cpu_online(i)) {
> - kgdb_info[i].debuggerinfo = NULL;
> - kgdb_info[i].task = NULL;
> - }
> - }
> if (ks->err_code == DIE_OOPS || reason == KDB_REASON_OOPS) {
> ks->pass_exception = 1;
> KDB_FLAG_SET(CATASTROPHIC);
> --
> 2.20.0.rc1.387.gf8505762e3-goog
>