From: Doug Anderson <dianders@chromium.org>
To: Petr Mladek <pmladek@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Matthias Kaehlcke <mka@chromium.org>,
kgdb-bugreport@lists.sourceforge.net,
Stephane Eranian <eranian@google.com>,
mpe@ellerman.id.au, Tzung-Bi Shih <tzungbi@chromium.org>,
Daniel Thompson <daniel.thompson@linaro.org>,
Mark Rutland <mark.rutland@arm.com>,
linuxppc-dev@lists.ozlabs.org, Sumit Garg <sumit.garg@linaro.org>,
npiggin@gmail.com, davem@davemloft.net,
Marc Zyngier <maz@kernel.org>, Stephen Boyd <swboyd@chromium.org>,
sparclinux@vger.kernel.org, christophe.leroy@csgroup.eu,
Catalin Marinas <catalin.marinas@arm.com>,
ravi.v.shankar@intel.com, Randy Dunlap <rdunlap@infradead.org>,
Pingfan Liu <kernelfans@gmail.com>,
Guenter Roeck <groeck@chromium.org>,
Lecopzer Chen <lecopzer.chen@mediatek.com>,
Ian Rogers <irogers@google.com>,
ito-yuichi@fujitsu.com, ricardo.neri@intel.com,
linux-arm-kernel@lists.infradead.org,
linux-perf-users@vger.kernel.org, Will Deacon <will@kernel.org>,
Chen-Yu Tsai <wens@csie.org>,
linux-kernel@vger.kernel.org,
Masayoshi Mizuma <msys.mizuma@gmail.com>,
Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v5 10/18] watchdog/hardlockup: Add a "cpu" param to watchdog_hardlockup_check()
Date: Tue, 23 May 2023 09:34:37 -0700 [thread overview]
Message-ID: <CAD=FV=VeGKTvw2=qhSqkSEtYwVrXGLNzNbgBAwrmn2CWWfJckQ@mail.gmail.com> (raw)
In-Reply-To: <ZGzjm9h85fpYZJMc@alley>
Hi,
On Tue, May 23, 2023 at 9:02 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2023-05-19 10:18:34, Douglas Anderson wrote:
> > In preparation for the buddy hardlockup detector where the CPU
> > checking for lockup might not be the currently running CPU, add a
> > "cpu" parameter to watchdog_hardlockup_check().
> >
> > As part of this change, make hrtimer_interrupts an atomic_t since now
> > the CPU incrementing the value and the CPU reading the value might be
> > different. Technially this could also be done with just READ_ONCE and
> > WRITE_ONCE, but atomic_t feels a little cleaner in this case.
> >
> > While hrtimer_interrupts is made atomic_t, we change
> > hrtimer_interrupts_saved from "unsigned long" to "int". The "int" is
> > needed to match the data type backing atomic_t for hrtimer_interrupts.
> > Even if this changes us from 64-bits to 32-bits (which I don't think
> > is true for most compilers), it doesn't really matter. All we ever do
> > is increment it every few seconds and compare it to an old value so
> > 32-bits is fine (even 16-bits would be). The "signed" vs "unsigned"
> > also doesn't matter for simple equality comparisons.
> >
> > hrtimer_interrupts_saved is _not_ switched to atomic_t nor even
> > accessed with READ_ONCE / WRITE_ONCE. The hrtimer_interrupts_saved is
> > always consistently accessed with the same CPU. NOTE: with the
> > upcoming "buddy" detector there is one special case. When a CPU goes
> > offline/online then we can change which CPU is the one to consistently
> > access a given instance of hrtimer_interrupts_saved. We still can't
> > end up with a partially updated hrtimer_interrupts_saved, however,
> > because we end up petting all affected CPUs to make sure the new and
> > old CPU can't end up somehow read/write hrtimer_interrupts_saved at
> > the same time.
> >
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -87,29 +87,34 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);
> >
> > #if defined(CONFIG_HARDLOCKUP_DETECTOR_PERF)
> >
> > -static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts);
> > -static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
> > +static DEFINE_PER_CPU(atomic_t, hrtimer_interrupts);
> > +static DEFINE_PER_CPU(int, hrtimer_interrupts_saved);
> > static DEFINE_PER_CPU(bool, watchdog_hardlockup_warned);
> > static unsigned long watchdog_hardlockup_all_cpu_dumped;
> >
> > -static bool is_hardlockup(void)
> > +static bool is_hardlockup(unsigned int cpu)
> > {
> > - unsigned long hrint = __this_cpu_read(hrtimer_interrupts);
> > + int hrint = atomic_read(&per_cpu(hrtimer_interrupts, cpu));
> >
> > - if (__this_cpu_read(hrtimer_interrupts_saved) == hrint)
> > + if (per_cpu(hrtimer_interrupts_saved, cpu) == hrint)
> > return true;
> >
> > - __this_cpu_write(hrtimer_interrupts_saved, hrint);
> > + /*
> > + * NOTE: we don't need any fancy atomic_t or READ_ONCE/WRITE_ONCE
> > + * for hrtimer_interrupts_saved. hrtimer_interrupts_saved is
> > + * written/read by a single CPU.
> > + */
> > + per_cpu(hrtimer_interrupts_saved, cpu) = hrint;
> >
> > return false;
> > }
> >
> > static void watchdog_hardlockup_kick(void)
> > {
> > - __this_cpu_inc(hrtimer_interrupts);
> > + atomic_inc(raw_cpu_ptr(&hrtimer_interrupts));
>
> Is there any particular reason why raw_*() is needed, please?
>
> My expectation is that the raw_ API should be used only when
> there is a good reason for it. Where a good reason might be
> when the checks might fail but the consistency is guaranteed
> another way.
>
> IMHO, we should use:
>
> atomic_inc(this_cpu_ptr(&hrtimer_interrupts));
>
> To be honest, I am a bit lost in the per_cpu API definitions.
>
> But this_cpu_ptr() seems to be implemented the same way as
> per_cpu_ptr() when CONFIG_DEBUG_PREEMPT is enabled.
> And we use per_cpu_ptr() in is_hardlockup().
>
> Also this_cpu_ptr() is used more commonly:
>
> $> git grep this_cpu_ptr | wc -l
> 1385
> $> git grep raw_cpu_ptr | wc -l
> 114
Hmmm, I think maybe I confused myself. The old code purposely used the
double-underscore prefixed version of this_cpu_inc(). I couldn't find
a double-underscore version of this_cpu_ptr() and I somehow convinced
myself that the raw() version was the right equivalent version.
You're right that this_cpu_ptr() is a better choice here and I don't
see any reason why we'd need the raw version.
> > }
> >
> > -void watchdog_hardlockup_check(struct pt_regs *regs)
> > +void watchdog_hardlockup_check(unsigned int cpu, struct pt_regs *regs)
> > {
> > /*
> > * Check for a hardlockup by making sure the CPU's timer
> > @@ -117,35 +122,42 @@ void watchdog_hardlockup_check(struct pt_regs *regs)
> > * fired multiple times before we overflow'd. If it hasn't
> > * then this is a good indication the cpu is stuck
> > */
> > - if (is_hardlockup()) {
> > + if (is_hardlockup(cpu)) {
> > unsigned int this_cpu = smp_processor_id();
> > + struct cpumask backtrace_mask = *cpu_online_mask;
>
> Does this work, please?
>
> IMHO, we should use cpumask_copy().
Ah, good call, thanks!
> > /* Only print hardlockups once. */
> > - if (__this_cpu_read(watchdog_hardlockup_warned))
> > + if (per_cpu(watchdog_hardlockup_warned, cpu))
> > return;
> >
>
> Otherwise, it looks good to me.
Neither change seems urgent though both are important to fix, I'll
wait a day or two to see if you have feedback on any of the other
patches and I'll send a fixup series.
-Doug
next prev parent reply other threads:[~2023-05-23 16:35 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-19 17:18 [PATCH v5 00/18] watchdog/hardlockup: Add the buddy hardlockup detector Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 01/18] watchdog/perf: Define dummy watchdog_update_hrtimer_threshold() on correct config Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 02/18] watchdog/perf: More properly prevent false positives with turbo modes Douglas Anderson
2023-05-23 9:35 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 03/18] watchdog: remove WATCHDOG_DEFAULT Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 04/18] watchdog/hardlockup: change watchdog_nmi_enable() to void Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 05/18] watchdog/perf: Ensure CPU-bound context when creating hardlockup detector event Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 06/18] watchdog/hardlockup: Add comments to touch_nmi_watchdog() Douglas Anderson
2023-05-23 9:58 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 07/18] watchdog/perf: Rename watchdog_hld.c to watchdog_perf.c Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 08/18] watchdog/hardlockup: Move perf hardlockup checking/panic to common watchdog.c Douglas Anderson
2023-05-23 11:45 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 09/18] watchdog/hardlockup: Style changes to watchdog_hardlockup_check() / is_hardlockup() Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 10/18] watchdog/hardlockup: Add a "cpu" param to watchdog_hardlockup_check() Douglas Anderson
2023-05-23 16:02 ` Petr Mladek
2023-05-23 16:34 ` Doug Anderson [this message]
2023-05-24 11:36 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 11/18] watchdog/hardlockup: Move perf hardlockup watchdog petting to watchdog.c Douglas Anderson
2023-05-24 13:07 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 12/18] watchdog/hardlockup: Rename some "NMI watchdog" constants/function Douglas Anderson
2023-05-24 13:38 ` Petr Mladek
2023-05-25 23:33 ` Doug Anderson
2023-05-19 17:18 ` [PATCH v5 13/18] watchdog/hardlockup: Have the perf hardlockup use __weak functions more cleanly Douglas Anderson
2023-05-24 13:59 ` Petr Mladek
2023-05-24 19:38 ` Doug Anderson
2023-05-26 14:44 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 14/18] watchdog/hardlockup: detect hard lockups using secondary (buddy) CPUs Douglas Anderson
2023-05-25 16:26 ` Petr Mladek
2023-05-25 20:08 ` Doug Anderson
2023-05-26 12:29 ` Petr Mladek
2023-05-19 17:18 ` [PATCH v5 15/18] watchdog/perf: Add a weak function for an arch to detect if perf can use NMIs Douglas Anderson
2023-05-26 12:36 ` Petr Mladek
2023-06-12 10:33 ` Mark Rutland
2023-06-12 13:55 ` Doug Anderson
2023-06-12 13:59 ` Mark Rutland
2023-05-19 17:18 ` [PATCH v5 16/18] watchdog/perf: Adapt the watchdog_perf interface for async model Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 17/18] arm64: add hw_nmi_get_sample_period for preparation of lockup detector Douglas Anderson
2023-05-19 17:18 ` [PATCH v5 18/18] arm64: Enable perf events based hard " Douglas Anderson
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='CAD=FV=VeGKTvw2=qhSqkSEtYwVrXGLNzNbgBAwrmn2CWWfJckQ@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=catalin.marinas@arm.com \
--cc=christophe.leroy@csgroup.eu \
--cc=daniel.thompson@linaro.org \
--cc=davem@davemloft.net \
--cc=eranian@google.com \
--cc=groeck@chromium.org \
--cc=irogers@google.com \
--cc=ito-yuichi@fujitsu.com \
--cc=kernelfans@gmail.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=lecopzer.chen@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=mka@chromium.org \
--cc=mpe@ellerman.id.au \
--cc=msys.mizuma@gmail.com \
--cc=npiggin@gmail.com \
--cc=pmladek@suse.com \
--cc=ravi.v.shankar@intel.com \
--cc=rdunlap@infradead.org \
--cc=ricardo.neri@intel.com \
--cc=sparclinux@vger.kernel.org \
--cc=sumit.garg@linaro.org \
--cc=swboyd@chromium.org \
--cc=tzungbi@chromium.org \
--cc=wens@csie.org \
--cc=will@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).