From: Sumit Garg <sumit.garg@linaro.org>
To: Doug Anderson <dianders@chromium.org>,
Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Daniel Thompson <daniel.thompson@linaro.org>,
linux-serial@vger.kernel.org,
kgdb-bugreport@lists.sourceforge.net,
Jiri Slaby <jslaby@suse.com>,
Russell King - ARM Linux <linux@armlinux.org.uk>,
Jason Wessel <jason.wessel@windriver.com>,
LKML <linux-kernel@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC 1/5] tty/sysrq: Make sysrq handler NMI aware
Date: Fri, 14 Aug 2020 12:54:35 +0530 [thread overview]
Message-ID: <CAFA6WYMy_+RdsPJekm7zmCrFUXHqjsfr3JvyD7L8A2X8+jB8Qw@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=Uqi28A=sm5+JhAqBM2OtBM3_XwvvkaKyEDVL9uVEioog@mail.gmail.com>
+ Peter (author of irq_work.c)
On Thu, 13 Aug 2020 at 05:30, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jul 21, 2020 at 5:10 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > In a future patch we will add support to the serial core to make it
> > possible to trigger a magic sysrq from an NMI context. Prepare for this
> > by marking some sysrq actions as NMI safe. Safe actions will be allowed
> > to run from NMI context whilst that cannot run from an NMI will be queued
> > as irq_work for later processing.
> >
> > A particular sysrq handler is only marked as NMI safe in case the handler
> > isn't contending for any synchronization primitives as in NMI context
> > they are expected to cause deadlocks. Note that the debug sysrq do not
> > contend for any synchronization primitives. It does call kgdb_breakpoint()
> > to provoke a trap but that trap handler should be NMI safe on
> > architectures that implement an NMI.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> > drivers/tty/sysrq.c | 33 ++++++++++++++++++++++++++++++++-
> > include/linux/sysrq.h | 1 +
> > kernel/debug/debug_core.c | 1 +
> > 3 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 7c95afa9..8017e33 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -50,6 +50,8 @@
> > #include <linux/syscalls.h>
> > #include <linux/of.h>
> > #include <linux/rcupdate.h>
> > +#include <linux/irq_work.h>
> > +#include <linux/kfifo.h>
> >
> > #include <asm/ptrace.h>
> > #include <asm/irq_regs.h>
> > @@ -111,6 +113,7 @@ static const struct sysrq_key_op sysrq_loglevel_op = {
> > .help_msg = "loglevel(0-9)",
> > .action_msg = "Changing Loglevel",
> > .enable_mask = SYSRQ_ENABLE_LOG,
> > + .nmi_safe = true,
> > };
> >
> > #ifdef CONFIG_VT
> > @@ -157,6 +160,7 @@ static const struct sysrq_key_op sysrq_crash_op = {
> > .help_msg = "crash(c)",
> > .action_msg = "Trigger a crash",
> > .enable_mask = SYSRQ_ENABLE_DUMP,
> > + .nmi_safe = true,
> > };
> >
> > static void sysrq_handle_reboot(int key)
> > @@ -170,6 +174,7 @@ static const struct sysrq_key_op sysrq_reboot_op = {
> > .help_msg = "reboot(b)",
> > .action_msg = "Resetting",
> > .enable_mask = SYSRQ_ENABLE_BOOT,
> > + .nmi_safe = true,
> > };
> >
> > const struct sysrq_key_op *__sysrq_reboot_op = &sysrq_reboot_op;
> > @@ -217,6 +222,7 @@ static const struct sysrq_key_op sysrq_showlocks_op = {
> > .handler = sysrq_handle_showlocks,
> > .help_msg = "show-all-locks(d)",
> > .action_msg = "Show Locks Held",
> > + .nmi_safe = true,
> > };
> > #else
> > #define sysrq_showlocks_op (*(const struct sysrq_key_op *)NULL)
> > @@ -289,6 +295,7 @@ static const struct sysrq_key_op sysrq_showregs_op = {
> > .help_msg = "show-registers(p)",
> > .action_msg = "Show Regs",
> > .enable_mask = SYSRQ_ENABLE_DUMP,
> > + .nmi_safe = true,
> > };
> >
> > static void sysrq_handle_showstate(int key)
> > @@ -326,6 +333,7 @@ static const struct sysrq_key_op sysrq_ftrace_dump_op = {
> > .help_msg = "dump-ftrace-buffer(z)",
> > .action_msg = "Dump ftrace buffer",
> > .enable_mask = SYSRQ_ENABLE_DUMP,
> > + .nmi_safe = true,
> > };
> > #else
> > #define sysrq_ftrace_dump_op (*(const struct sysrq_key_op *)NULL)
> > @@ -538,6 +546,23 @@ static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p)
> > sysrq_key_table[i] = op_p;
> > }
> >
> > +#define SYSRQ_NMI_FIFO_SIZE 64
> > +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE);
>
> A 64-entry FIFO seems excessive. Quite honestly even a FIFO seems a
> bit excessive and it feels like if two sysrqs were received in super
> quick succession that it would be OK to just process the first one. I
> guess if it simplifies the processing to have a FIFO then it shouldn't
> hurt, but no need for 64 entries.
>
Okay, would a 2-entry FIFO work here? As here we need a FIFO to pass
on the key parameter.
>
> > +static void sysrq_do_nmi_work(struct irq_work *work)
> > +{
> > + const struct sysrq_key_op *op_p;
> > + int key;
> > +
> > + while (kfifo_out(&sysrq_nmi_fifo, &key, 1)) {
> > + op_p = __sysrq_get_key_op(key);
> > + if (op_p)
> > + op_p->handler(key);
> > + }
>
> Do you need to manage "suppress_printk" in this function? Do you need
> to call rcu_sysrq_start() and rcu_read_lock()?
Ah I missed those. Will add them here instead.
>
> If so, how do you prevent racing between the mucking we're doing with
> these things and the mucking that the NMI does with them?
IIUC, here you meant to highlight the race while scheduled sysrq is
executing in IRQ context and we receive a new sysrq in NMI context,
correct? If yes, this seems to be a trickier situation. I think the
appropriate way to handle it would be to deny any further sysrq
handling until the prior sysrq handling is complete, your views?
>
>
> > +}
> > +
> > +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work);
> > +
> > void __handle_sysrq(int key, bool check_mask)
> > {
> > const struct sysrq_key_op *op_p;
> > @@ -568,7 +593,13 @@ void __handle_sysrq(int key, bool check_mask)
> > if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> > pr_info("%s\n", op_p->action_msg);
> > console_loglevel = orig_log_level;
> > - op_p->handler(key);
> > +
> > + if (in_nmi() && !op_p->nmi_safe) {
> > + kfifo_in(&sysrq_nmi_fifo, &key, 1);
>
> Rather than kfifo_in() and kfifo_out(), I think you can use
> kfifo_put() and kfifo_get(). As I understand it those just get/put
> one element which is what you want.
Okay, will use kfifo_put() and kfifo_get() here instead.
>
>
> > + irq_work_queue(&sysrq_nmi_work);
>
> Wishful thinking, but (as far as I can tell) irq_work_queue() only
> queues work on the CPU running the NMI. I don't have lots of NMI
> experience, but any chance there is a variant that will queue work on
> any CPU? Then sysrq handlers that aren't NMI aware will be more
> likely to work.
>
Unfortunately, queuing work on other CPUs isn't safe in NMI context,
see this warning [1]. The comment mentions:
/* Arch remote IPI send/receive backend aren't NMI safe */
Peter,
Can you throw some light here as to why it isn't considered NMI-safe
to send remote IPI in NMI context? Is it an arch specific limitation?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq_work.c#n103
-Sumit
>
>
>
> > + } else {
> > + op_p->handler(key);
> > + }
> > } else {
> > pr_info("This sysrq operation is disabled.\n");
> > console_loglevel = orig_log_level;
> > diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
> > index 3a582ec..630b5b9 100644
> > --- a/include/linux/sysrq.h
> > +++ b/include/linux/sysrq.h
> > @@ -34,6 +34,7 @@ struct sysrq_key_op {
> > const char * const help_msg;
> > const char * const action_msg;
> > const int enable_mask;
> > + const bool nmi_safe;
> > };
> >
> > #ifdef CONFIG_MAGIC_SYSRQ
> > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> > index 9e59347..2b51173 100644
> > --- a/kernel/debug/debug_core.c
> > +++ b/kernel/debug/debug_core.c
> > @@ -943,6 +943,7 @@ static const struct sysrq_key_op sysrq_dbg_op = {
> > .handler = sysrq_handle_dbg,
> > .help_msg = "debug(g)",
> > .action_msg = "DEBUG",
> > + .nmi_safe = true,
> > };
> > #endif
> >
> > --
> > 2.7.4
> >
next prev parent reply other threads:[~2020-08-14 7:24 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-21 12:10 [RFC 0/5] Introduce NMI aware serial drivers Sumit Garg
2020-07-21 12:10 ` [RFC 1/5] tty/sysrq: Make sysrq handler NMI aware Sumit Garg
2020-08-12 23:59 ` Doug Anderson
2020-08-14 7:24 ` Sumit Garg [this message]
2020-08-14 14:34 ` peterz
2020-08-14 14:57 ` Doug Anderson
2020-08-17 14:08 ` Sumit Garg
2020-08-17 17:19 ` Doug Anderson
2020-08-18 13:30 ` Sumit Garg
2020-07-21 12:10 ` [RFC 2/5] serial: core: Add framework to allow NMI aware serial drivers Sumit Garg
2020-08-12 23:59 ` Doug Anderson
2020-08-13 14:19 ` Sumit Garg
2020-08-13 14:37 ` Doug Anderson
2020-08-14 11:17 ` Sumit Garg
2020-08-14 14:13 ` Daniel Thompson
2020-08-17 12:27 ` Sumit Garg
2020-08-17 13:57 ` Doug Anderson
2020-08-17 14:23 ` Sumit Garg
2020-08-17 14:32 ` Daniel Thompson
2020-08-18 13:18 ` Sumit Garg
2020-08-17 14:28 ` Daniel Thompson
2020-08-18 13:06 ` Sumit Garg
2020-08-14 14:43 ` Doug Anderson
2020-08-17 12:29 ` Sumit Garg
2020-07-21 12:10 ` [RFC 3/5] serial: amba-pl011: Re-order APIs definition Sumit Garg
2020-07-21 12:10 ` [RFC 4/5] serial: amba-pl011: Enable NMI aware uart port Sumit Garg
2020-08-12 23:59 ` Doug Anderson
2020-08-13 10:34 ` Sumit Garg
2020-07-21 12:10 ` [RFC 5/5] serial: Remove KGDB NMI serial driver Sumit Garg
2020-08-11 13:50 ` [RFC 0/5] Introduce NMI aware serial drivers Sumit Garg
2020-08-11 13:58 ` Greg Kroah-Hartman
2020-08-11 14:29 ` Sumit Garg
2020-08-11 14:58 ` Greg Kroah-Hartman
2020-08-11 17:15 ` Doug Anderson
2020-08-12 14:52 ` Sumit Garg
2020-08-12 15:27 ` Doug Anderson
2020-08-13 0:08 ` Doug Anderson
2020-08-13 9:25 ` Sumit Garg
2020-08-13 10:17 ` Daniel Thompson
2020-08-14 12:06 ` Sumit Garg
2020-08-14 14:18 ` Daniel Thompson
2020-08-17 5:12 ` Sumit Garg
2020-08-17 9:28 ` Daniel Thompson
2020-08-17 14:12 ` Sumit Garg
2020-08-13 15:26 ` Doug Anderson
2020-08-14 12:50 ` Sumit Garg
2020-08-12 5:48 ` Sumit Garg
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=CAFA6WYMy_+RdsPJekm7zmCrFUXHqjsfr3JvyD7L8A2X8+jB8Qw@mail.gmail.com \
--to=sumit.garg@linaro.org \
--cc=daniel.thompson@linaro.org \
--cc=dianders@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=jason.wessel@windriver.com \
--cc=jslaby@suse.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=peterz@infradead.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).