Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231114021529.1834-1-lizhe.67@bytedance.com>
In-Reply-To: <20231114021529.1834-1-lizhe.67@bytedance.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 17 Nov 2023 13:45:21 -0800
Message-ID: <CAD=FV=XhqDxmk=yTdujwtFyF23NZ13LORH0GMS5_iTAEVva_rA@mail.gmail.com>
Subject: Re: [RFC] softlockup: serialized softlockup's log
To:     lizhe.67@bytedance.com
Cc:     akpm@linux-foundation.org, pmladek@suse.com, kernelfans@gmail.com,
        lecopzer.chen@mediatek.com, linux-kernel@vger.kernel.org,
        lizefan.x@bytedance.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Mon, Nov 13, 2023 at 6:16=E2=80=AFPM <lizhe.67@bytedance.com> wrote:
>
> From: Li Zhe <lizhe.67@bytedance.com>
>
> If multiple CPUs trigger softlockup at the same time, the softlockup's
> logs will appear staggeredly in dmesg, which will affect the viewing of
> the logs for developer. Since the code path for outputting softlockup log=
s
> is not a kernel hotspot and the performance requirements for the code
> are not strict, locks are used to serialize the softlockup log output
> to improve the readability of the logs.
>
> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
> ---
>  kernel/watchdog.c | 3 +++
>  1 file changed, 3 insertions(+)

This seems reasonable to me. It might be interesting to talk about in
your commit message how this interacts with the various options. From
code inspection, I believe:

* If `softlockup_all_cpu_backtrace` then this is a no-op since other
CPUs will be prevented from running the printing code while one is
already printing.

* I'm not 100% sure what happens if `softlockup_panic` is set and I
haven't sat down to test this myself. Will one CPUs panic message
interleave the other CPUs traces. I guess in the end both CPUs will
call panic()? Maybe you could experiment and describe the behavior in
your commit message?


> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 5cd6d4e26915..8324ac194d0a 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -448,6 +448,7 @@ static enum hrtimer_restart watchdog_timer_fn(struct =
hrtimer *hrtimer)
>         struct pt_regs *regs =3D get_irq_regs();
>         int duration;
>         int softlockup_all_cpu_backtrace =3D sysctl_softlockup_all_cpu_ba=
cktrace;
> +       static DEFINE_SPINLOCK(watchdog_timer_lock);

I'd be tempted to define this outside the scope of this function. I
need to dig more, but I'm pretty sure I've seen cases where a soft
lockup could trigger while I was trying to print traces for a
hardlockup, so it might be useful to grab the same spinlock in both
places...