Skip to contentSkip to conversation

serial: core: fix up sysrq regressions

PATCH In review Johan Hovold 4 participants 12 messages Jun 10, 2020 – Jun 27, 2020 TTY layer and serial drivers

lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover serial: core: fix up sysrq regressions

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(-)
Signature · 1 line
2.26.2
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/tty/serial/serial_core.c | 23 +++++++++++++----------
 include/linux/serial_core.h      |  3 ++-
 2 files changed, 15 insertions(+), 11 deletions(-)
drivers/tty/serial/serial_core.c +13 -10
index 57840cf90388..296b9f6fa9cd 100644
@@ -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);
include/linux/serial_core.h +2 -1
index 9fd550e7946a..ef4921ddbe97 100644
@@ -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);
 
 /*
Signature · 1 line
2.26.2
author
#2 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serial/serial_core.c |  99 +----------------------------
 include/linux/serial_core.h      | 103 +++++++++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 102 deletions(-)
drivers/tty/serial/serial_core.c +2 -97
index 296b9f6fa9cd..3706f31b0c37 100644
@@ -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);
include/linux/serial_core.h +98 -5

109 lines not on this page · raw message

Signature · 1 line
2.26.2
#3 lore raw

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

Hi Johan,

Quoted 5 lines · 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.

The part with ifdeffing looks good to me.

Quoted 3 lines

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

author
#4 lore raw

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

Quoted 9 lines · Dmitry Safonov

Hi Johan,

Quoted 5 lines · 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.

Quoted 8 lines

The part with ifdeffing looks good to me.

Quoted 3 lines

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.

Quoted 2 lines

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

#5 lore raw

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

Quoted 16 lines · Johan Hovold
Quoted 15 lines · Dmitry Safonov

Hi Johan,

Quoted 5 lines · 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.

The part with ifdeffing looks good to me.

Quoted 3 lines

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).

Quoted 5 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines

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

Reviewed-by on v2 2/3
#6 lore raw

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

In reply to Johan Hovold
Quoted 6 lines · 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.

Quoted 7 lines

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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

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 <[email protected]>

Thanks,
Dmitry

author
#7 lore raw

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

In reply to 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 <[email protected]>
---
 include/linux/serial_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/serial_core.h +2 -2
index 03fa7b967103..791f4844efeb 100644
@@ -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)) {
Signature · 1 line
2.26.2
Reviewed-by on v2 3/3
#8 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Dmitry Safonov <[email protected]>

Thanks,
Dmitry

Reviewed-by on the v2 series
#9 lore raw
In reply to Johan Hovold
Quoted 6 lines · 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).

I see a problem, thanks for pointing out!
The fix LGTM! FWIW,

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 20 lines

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(-)
Signature · 1 line
2.26.2
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#10 lore raw
Quoted 11 lines · Andy Shevchenko
Quoted 6 lines · 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).

I see a problem, thanks for pointing out!
The fix LGTM! FWIW,

Reviewed-by Andy Shevchenko <[email protected]>

Thanks for reviewing, Andy.

Johan

maintainer
#11 lore raw
In reply to Johan Hovold
Quoted 6 lines · 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).

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

greg k-h