[PATCH v2 0/3] serial: core: fix up sysrq regressions

[PATCH v2 0/3] serial: core: fix up sysrq regressions

[Johan Hovold]

This series fixes a few regressions introduced by the recent sysrq
rework that went into 5.6.

The fix for the unnecessary per-character overhead probably could have
been marked for stable but I left that decision to the maintainers as it
is a bit intrusive (although mostly shuffling code around).

Johan

Changes in v2
 - inline uart_unlock_and_check_sysrq() along with the other helpers
   (restoring the interrupt state in a helper was never an issue)


Johan Hovold (3):
  Revert "serial: core: Refactor uart_unlock_and_check_sysrq()"
  serial: core: fix sysrq overhead regression
  serial: core: drop redundant sysrq checks

 drivers/tty/serial/serial_core.c |  96 +----------------------------
 include/linux/serial_core.h      | 102 +++++++++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 98 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] Revert "serial: core: Refactor uart_unlock_and_check_sysrq()"

[Johan Hovold]

This reverts commit da9a5aa3402db0ff3b57216d8dbf2478e1046cae.

In order to ease backporting a fix for a sysrq regression, revert this
rewrite which was since added on top.

The other sysrq helpers now bail out early when sysrq is not enabled;
it's better to keep that pattern here as well.

Note that the __releases() attribute won't be needed after the follow-on
fix either.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 23 +++++++++++++----------
 include/linux/serial_core.h      |  3 ++-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 57840cf90388..296b9f6fa9cd 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3239,19 +3239,22 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 }
 EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
 
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
-__releases(&port->lock)
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
 {
-	if (port->has_sysrq) {
-		int sysrq_ch = port->sysrq_ch;
+	int sysrq_ch;
 
-		port->sysrq_ch = 0;
-		spin_unlock_irqrestore(&port->lock, flags);
-		if (sysrq_ch)
-			handle_sysrq(sysrq_ch);
-	} else {
-		spin_unlock_irqrestore(&port->lock, flags);
+	if (!port->has_sysrq) {
+		spin_unlock_irqrestore(&port->lock, irqflags);
+		return;
 	}
+
+	sysrq_ch = port->sysrq_ch;
+	port->sysrq_ch = 0;
+
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	if (sysrq_ch)
+		handle_sysrq(sysrq_ch);
 }
 EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9fd550e7946a..ef4921ddbe97 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -464,7 +464,8 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
 
 extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
 extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
-extern void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port,
+					unsigned long irqflags);
 extern int uart_handle_break(struct uart_port *port);
 
 /*
-- 
2.26.2

[PATCH v2 2/3] serial: core: fix sysrq overhead regression

[Johan Hovold]

Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
file") converted the inline sysrq helpers to exported functions which
are now called for every received character, interrupt and break signal
also on systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being
optimised away by the compiler.

Inlining these helpers again also avoids the function call overhead when
CONFIG_MAGIC_SYSRQ_SERIAL is enabled (e.g. when the port is not used as
a console).

Fixes: 8e20fc391711 ("serial_core: Move sysrq functions from header file")
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c |  99 +----------------------------
 include/linux/serial_core.h      | 103 +++++++++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 102 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 296b9f6fa9cd..3706f31b0c37 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -41,8 +41,6 @@ static struct lock_class_key port_lock_key;
 
 #define HIGH_BITS_OFFSET	((sizeof(long)-sizeof(int))*8)
 
-#define SYSRQ_TIMEOUT	(HZ * 5)
-
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios);
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
@@ -3163,7 +3161,7 @@ static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
  *	Returns false if @ch is out of enabling sequence and should be
  *	handled some other way, true if @ch was consumed.
  */
-static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 {
 	int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
 
@@ -3186,102 +3184,9 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 	port->sysrq = 0;
 	return true;
 }
-#else
-static inline bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
-{
-	return false;
-}
+EXPORT_SYMBOL_GPL(uart_try_toggle_sysrq);
 #endif
 
-int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
-{
-	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
-		return 0;
-
-	if (!port->has_sysrq || !port->sysrq)
-		return 0;
-
-	if (ch && time_before(jiffies, port->sysrq)) {
-		if (sysrq_mask()) {
-			handle_sysrq(ch);
-			port->sysrq = 0;
-			return 1;
-		}
-		if (uart_try_toggle_sysrq(port, ch))
-			return 1;
-	}
-	port->sysrq = 0;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(uart_handle_sysrq_char);
-
-int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
-{
-	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
-		return 0;
-
-	if (!port->has_sysrq || !port->sysrq)
-		return 0;
-
-	if (ch && time_before(jiffies, port->sysrq)) {
-		if (sysrq_mask()) {
-			port->sysrq_ch = ch;
-			port->sysrq = 0;
-			return 1;
-		}
-		if (uart_try_toggle_sysrq(port, ch))
-			return 1;
-	}
-	port->sysrq = 0;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
-
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
-{
-	int sysrq_ch;
-
-	if (!port->has_sysrq) {
-		spin_unlock_irqrestore(&port->lock, irqflags);
-		return;
-	}
-
-	sysrq_ch = port->sysrq_ch;
-	port->sysrq_ch = 0;
-
-	spin_unlock_irqrestore(&port->lock, irqflags);
-
-	if (sysrq_ch)
-		handle_sysrq(sysrq_ch);
-}
-EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
-
-/*
- * We do the SysRQ and SAK checking like this...
- */
-int uart_handle_break(struct uart_port *port)
-{
-	struct uart_state *state = port->state;
-
-	if (port->handle_break)
-		port->handle_break(port);
-
-	if (port->has_sysrq && uart_console(port)) {
-		if (!port->sysrq) {
-			port->sysrq = jiffies + SYSRQ_TIMEOUT;
-			return 1;
-		}
-		port->sysrq = 0;
-	}
-
-	if (port->flags & UPF_SAK)
-		do_SAK(state->port.tty);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(uart_handle_break);
-
 EXPORT_SYMBOL(uart_write_wakeup);
 EXPORT_SYMBOL(uart_register_driver);
 EXPORT_SYMBOL(uart_unregister_driver);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index ef4921ddbe97..03fa7b967103 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -462,11 +462,104 @@ extern void uart_handle_cts_change(struct uart_port *uport,
 extern void uart_insert_char(struct uart_port *port, unsigned int status,
 		 unsigned int overrun, unsigned int ch, unsigned int flag);
 
-extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
-extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
-extern void uart_unlock_and_check_sysrq(struct uart_port *port,
-					unsigned long irqflags);
-extern int uart_handle_break(struct uart_port *port);
+#ifdef CONFIG_MAGIC_SYSRQ_SERIAL
+#define SYSRQ_TIMEOUT	(HZ * 5)
+
+bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch);
+
+static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		if (sysrq_mask()) {
+			handle_sysrq(ch);
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
+
+static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		if (sysrq_mask()) {
+			port->sysrq_ch = ch;
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
+
+static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+{
+	int sysrq_ch;
+
+	if (!port->has_sysrq) {
+		spin_unlock_irqrestore(&port->lock, irqflags);
+		return;
+	}
+
+	sysrq_ch = port->sysrq_ch;
+	port->sysrq_ch = 0;
+
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	if (sysrq_ch)
+		handle_sysrq(sysrq_ch);
+}
+#else	/* CONFIG_MAGIC_SYSRQ_SERIAL */
+static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	return 0;
+}
+static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	return 0;
+}
+static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+{
+	spin_unlock_irqrestore(&port->lock, irqflags);
+}
+#endif	/* CONFIG_MAGIC_SYSRQ_SERIAL */
+
+/*
+ * We do the SysRQ and SAK checking like this...
+ */
+static inline int uart_handle_break(struct uart_port *port)
+{
+	struct uart_state *state = port->state;
+
+	if (port->handle_break)
+		port->handle_break(port);
+
+#ifdef CONFIG_MAGIC_SYSRQ_SERIAL
+	if (port->has_sysrq && uart_console(port)) {
+		if (!port->sysrq) {
+			port->sysrq = jiffies + SYSRQ_TIMEOUT;
+			return 1;
+		}
+		port->sysrq = 0;
+	}
+#endif
+	if (port->flags & UPF_SAK)
+		do_SAK(state->port.tty);
+	return 0;
+}
 
 /*
  *	UART_ENABLE_MS - determine if port should enable modem status irqs
-- 
2.26.2

[PATCH v2 3/3] serial: core: drop redundant sysrq checks

[Johan Hovold]

The sysrq timestamp will never be set unless port->has_sysrq is set (see
uart_handle_break()) so drop the redundant checks that were added by
commit 1997e9dfdc84 ("serial_core: Un-ifdef sysrq SUPPORT_SYSRQ").

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/serial_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 03fa7b967103..791f4844efeb 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -469,7 +469,7 @@ bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch);
 
 static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
-	if (!port->has_sysrq || !port->sysrq)
+	if (!port->sysrq)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
@@ -488,7 +488,7 @@ static inline int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch
 
 static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 {
-	if (!port->has_sysrq || !port->sysrq)
+	if (!port->sysrq)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
-- 
2.26.2

Re: [PATCH v2 0/3] serial: core: fix up sysrq regressions

[Andy Shevchenko]

On Wed, Jun 10, 2020 at 05:22:29PM +0200, Johan Hovold wrote:
> This series fixes a few regressions introduced by the recent sysrq
> rework that went into 5.6.
> 
> The fix for the unnecessary per-character overhead probably could have
> been marked for stable but I left that decision to the maintainers as it
> is a bit intrusive (although mostly shuffling code around).

I see a problem, thanks for pointing out!
The fix LGTM! FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Johan
> 
> Changes in v2
>  - inline uart_unlock_and_check_sysrq() along with the other helpers
>    (restoring the interrupt state in a helper was never an issue)
> 
> 
> Johan Hovold (3):
>   Revert "serial: core: Refactor uart_unlock_and_check_sysrq()"
>   serial: core: fix sysrq overhead regression
>   serial: core: drop redundant sysrq checks
> 
>  drivers/tty/serial/serial_core.c |  96 +----------------------------
>  include/linux/serial_core.h      | 102 +++++++++++++++++++++++++++++--
>  2 files changed, 100 insertions(+), 98 deletions(-)
> 
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] serial: core: fix sysrq overhead regression

[Dmitry Safonov]

Hi Johan,

On 6/10/20 4:22 PM, Johan Hovold wrote:
> Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
> file") converted the inline sysrq helpers to exported functions which
> are now called for every received character, interrupt and break signal
> also on systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being
> optimised away by the compiler.

The part with ifdeffing looks good to me.

> Inlining these helpers again also avoids the function call overhead when
> CONFIG_MAGIC_SYSRQ_SERIAL is enabled (e.g. when the port is not used as
> a console).

But this one, coul you add measures? (it will also help to understand if
it's a stable material).

If one function call actually matters here, than should
uart_insert_char() also go into header?

I see quite common pattern in drivers:
: if (!uart_handle_sysrq_char(&up->port, ch))
: 	uart_insert_char(&up->port, byte, 0, ch, TTY_NORMAL);

Don't misunderstand me, but I would prefer keeping headers cleaner
without realization details with the exception if function calls
actually hurts the performance.

Probably, a comment like
/*
 * Keeping these functions in the header improves performance by X% on
 * YYY platform by letting the compiler inline them.
 */
would also help.

Thanks for working on this,
          Dmitry

Re: [PATCH v2 2/3] serial: core: fix sysrq overhead regression

[Johan Hovold]

On Wed, Jun 10, 2020 at 05:24:57PM +0100, Dmitry Safonov wrote:
> Hi Johan,
> 
> On 6/10/20 4:22 PM, Johan Hovold wrote:
> > Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
> > file") converted the inline sysrq helpers to exported functions which
> > are now called for every received character, interrupt and break signal
> > also on systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being
> > optimised away by the compiler.
> 
> The part with ifdeffing looks good to me.
> 
> > Inlining these helpers again also avoids the function call overhead when
> > CONFIG_MAGIC_SYSRQ_SERIAL is enabled (e.g. when the port is not used as
> > a console).
> 
> But this one, coul you add measures? (it will also help to understand if
> it's a stable material).

Interrupt processing takes 2-3% longer without the inlining with
8250_omap on a beagleboard for example.

> If one function call actually matters here, than should
> uart_insert_char() also go into header?

Good question, it actually was originally intended to be inlined as all
other per-character processing. Separate discussion though.

The point is that we don't want a rarely used debugging feature to incur
unnecessary additional overhead that can easily be avoided.

Johan

Re: [PATCH v2 0/3] serial: core: fix up sysrq regressions

[Johan Hovold]

On Wed, Jun 10, 2020 at 07:21:34PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 10, 2020 at 05:22:29PM +0200, Johan Hovold wrote:
> > This series fixes a few regressions introduced by the recent sysrq
> > rework that went into 5.6.
> > 
> > The fix for the unnecessary per-character overhead probably could have
> > been marked for stable but I left that decision to the maintainers as it
> > is a bit intrusive (although mostly shuffling code around).
> 
> I see a problem, thanks for pointing out!
> The fix LGTM! FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for reviewing, Andy.

Johan

Re: [PATCH v2 2/3] serial: core: fix sysrq overhead regression

[Dmitry Safonov]

On 6/12/20 4:29 PM, Johan Hovold wrote:
> On Wed, Jun 10, 2020 at 05:24:57PM +0100, Dmitry Safonov wrote:
>> Hi Johan,
>>
>> On 6/10/20 4:22 PM, Johan Hovold wrote:
>>> Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
>>> file") converted the inline sysrq helpers to exported functions which
>>> are now called for every received character, interrupt and break signal
>>> also on systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being
>>> optimised away by the compiler.
>>
>> The part with ifdeffing looks good to me.
>>
>>> Inlining these helpers again also avoids the function call overhead when
>>> CONFIG_MAGIC_SYSRQ_SERIAL is enabled (e.g. when the port is not used as
>>> a console).
>>
>> But this one, coul you add measures? (it will also help to understand if
>> it's a stable material).
> 
> Interrupt processing takes 2-3% longer without the inlining with
> 8250_omap on a beagleboard for example.

I think the number justifies moving them back to header.

> 
>> If one function call actually matters here, than should
>> uart_insert_char() also go into header?
> 
> Good question, it actually was originally intended to be inlined as all
> other per-character processing. Separate discussion though.

Fair enough

> The point is that we don't want a rarely used debugging feature to incur
> unnecessary additional overhead that can easily be avoided.

Well, it wasn't related to the debug feature, rather I wanted to cleanup
the header from a bit over-grown functions those have realization
details. And couldn't foresee that a function call would matter for some
setup.

Thanks,
           Dmitry

Re: [PATCH v2 2/3] serial: core: fix sysrq overhead regression

[Dmitry Safonov]

On 6/10/20 4:22 PM, Johan Hovold wrote:
> Commit 8e20fc391711 ("serial_core: Move sysrq functions from header
> file") converted the inline sysrq helpers to exported functions which
> are now called for every received character, interrupt and break signal
> also on systems without CONFIG_MAGIC_SYSRQ_SERIAL instead of being
> optimised away by the compiler.
> 
> Inlining these helpers again also avoids the function call overhead when
> CONFIG_MAGIC_SYSRQ_SERIAL is enabled (e.g. when the port is not used as
> a console).
> 
> Fixes: 8e20fc391711 ("serial_core: Move sysrq functions from header file")
> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Thanks for sending and the numbers, it's a bit pity that we need to move
them back to the header, but as it matters for your setup,

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

Thanks,
           Dmitry

Re: [PATCH v2 3/3] serial: core: drop redundant sysrq checks

[Dmitry Safonov]

On 6/10/20 4:22 PM, Johan Hovold wrote:
> The sysrq timestamp will never be set unless port->has_sysrq is set (see
> uart_handle_break()) so drop the redundant checks that were added by
> commit 1997e9dfdc84 ("serial_core: Un-ifdef sysrq SUPPORT_SYSRQ").
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

Thanks,
          Dmitry

Re: [PATCH v2 0/3] serial: core: fix up sysrq regressions

[Greg Kroah-Hartman]

On Wed, Jun 10, 2020 at 05:22:29PM +0200, Johan Hovold wrote:
> This series fixes a few regressions introduced by the recent sysrq
> rework that went into 5.6.
> 
> The fix for the unnecessary per-character overhead probably could have
> been marked for stable but I left that decision to the maintainers as it
> is a bit intrusive (although mostly shuffling code around).

It makes sense to backport this, I'll mark it as such, thanks!

greg k-h