diff mbox series

[printk,v2,16/26] printk: nbcon: Use nbcon consoles in console_flush_all()

Message ID 20240218185726.1994771-17-john.ogness@linutronix.de
State New
Headers
Series wire up write_atomic() printing |

Commit Message

John Ogness Feb. 18, 2024, 6:57 p.m. UTC 
  Allow nbcon consoles to print messages in the legacy printk()
caller context (printing via unlock) by integrating them into
console_flush_all(). The write_atomic() callback is used for
printing.

Provide nbcon_legacy_emit_next_record(), which acts as the
nbcon variant of console_emit_next_record(). Call this variant
within console_flush_all() for nbcon consoles. Since nbcon
consoles use their own @nbcon_seq variable to track the next
record to print, this also must be appropriately handled.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/internal.h |  6 ++++++
 kernel/printk/nbcon.c    | 45 ++++++++++++++++++++++++++++++++++++++++
 kernel/printk/printk.c   | 19 ++++++++++++-----
 3 files changed, 65 insertions(+), 5 deletions(-)

Comments

Petr Mladek Feb. 23, 2024, 5:15 p.m. UTC | #1 
On Sun 2024-02-18 20:03:16, John Ogness wrote:
> Allow nbcon consoles to print messages in the legacy printk()
> caller context (printing via unlock) by integrating them into
> console_flush_all(). The write_atomic() callback is used for
> printing.
> 
> Provide nbcon_legacy_emit_next_record(), which acts as the
> nbcon variant of console_emit_next_record(). Call this variant
> within console_flush_all() for nbcon consoles. Since nbcon
> consoles use their own @nbcon_seq variable to track the next
> record to print, this also must be appropriately handled.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -960,6 +961,50 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
>  	return ctxt->backlog;
>  }
>  
> +/**
> + * nbcon_legacy_emit_next_record - Print one record for an nbcon console
> + *					in legacy contexts
> + * @con:	The console to print on
> + * @handover:	Will be set to true if a printk waiter has taken over the
> + *		console_lock, in which case the caller is no longer holding
> + *		both the console_lock and the SRCU read lock. Otherwise it
> + *		is set to false.
> + * @cookie:	The cookie from the SRCU read lock.
> + *
> + * Context:	Any context which could not be migrated to another CPU.

This seems to be a relic after shufling the code. IMHO, it was meant
for nbcon_atomic_emit_one(). This function disables interrupts
to prevent CPU migration when calling nbcon_atomic_emit_one().

> + * Return:	True if a record could be printed, otherwise false.

A more precise might be to say "has been printed" instead of
"could be printed".

It is more problematic here. It could return false also when it was
not able to acquire nbcon console.

While console_emit_next_record() has a bit different semantic.
It returns false only when there is no new record.

Thinking loudly:

It probably is not a bit deal. console_flush_all() does not guarantee
that it flushed all messages. It might return early when
the console_lock was handovered. It means there there is
another legacy context flushing the messages.

And nbcon console could not be acquired here also when
there is another nbcon context already flushing the nbcon.

Another question is whether console_flush_all() should return "false"
when nbcon consoles were not flushed because they could not be acquired.
It might be needed so that console_unlock() does not wait until
the other context flushes the nbcon console.

> + *
> + * This function is meant to be called by console_flush_all() to print records
> + * on nbcon consoles from legacy context (printing via console unlocking).
> + * Essentially it is the nbcon version of console_emit_next_record().
> + */
> +bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> +				   int cookie)
> +{
> +	struct nbcon_write_context wctxt = { };
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> +	bool progress = false;
> +	unsigned long flags;
> +
> +	*handover = false;
> +
> +	/* Use the same procedure as console_emit_next_record(). */
> +	printk_safe_enter_irqsave(flags);
> +	console_lock_spinning_enable();
> +	stop_critical_timings();
> +
> +	ctxt->console	= con;
> +	ctxt->prio	= NBCON_PRIO_NORMAL;
> +
> +	progress = nbcon_atomic_emit_one(&wctxt);
> +
> +	start_critical_timings();
> +	*handover = console_lock_spinning_disable_and_check(cookie);
> +	printk_safe_exit_irqrestore(flags);
> +
> +	return progress;
> +}
> +
>  /**
>   * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
>   *					write_atomic() callback
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2948,13 +2948,22 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>  		cookie = console_srcu_read_lock();
>  		for_each_console_srcu(con) {
>  			short flags = console_srcu_read_flags(con);
> +			u64 printk_seq;
>  			bool progress;
>  
>  			if (!console_is_usable(con, flags))
>  				continue;
>  			any_usable = true;
>  
> -			progress = console_emit_next_record(con, handover, cookie);
> +			if (flags & CON_NBCON) {
> +				progress = nbcon_legacy_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = nbcon_seq_read(con);

It might looks better without the empty line. But it is just my view.
Feel free to keep it as is.

> +			} else {
> +				progress = console_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = con->seq;
> +			}
>  
>  			/*
>  			 * If a handover has occurred, the SRCU read lock

Best Regards,
Petr
diff mbox series

Patch

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index d9a5205692fc..b34847ec6b0d 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -71,6 +71,8 @@  void defer_console_output(void);
 
 u16 printk_parse_prefix(const char *text, int *level,
 			enum printk_info_flags *flags);
+void console_lock_spinning_enable(void);
+int console_lock_spinning_disable_and_check(int cookie);
 
 u64 nbcon_seq_read(struct console *con);
 void nbcon_seq_force(struct console *con, u64 seq);
@@ -78,6 +80,8 @@  bool nbcon_alloc(struct console *con);
 void nbcon_init(struct console *con);
 void nbcon_free(struct console *con);
 void nbcon_atomic_flush_all(void);
+bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+				   int cookie);
 
 /*
  * Check if the given console is currently capable and allowed to print
@@ -133,6 +137,8 @@  static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
 static inline void nbcon_atomic_flush_all(void) { }
+static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+						 int cookie) { return false; }
 
 static inline bool console_is_usable(struct console *con, short flags) { return false; }
 
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 2eb2929c1027..747f5cbfe5ee 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -532,6 +532,7 @@  static struct printk_buffers panic_nbcon_pbufs;
  * nbcon_context_try_acquire - Try to acquire nbcon console
  * @ctxt:	The context of the caller
  *
+ * Context:	Any context which could not be migrated to another CPU.
  * Return:	True if the console was acquired. False otherwise.
  *
  * If the caller allowed an unsafe hostile takeover, on success the
@@ -960,6 +961,50 @@  static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
 	return ctxt->backlog;
 }
 
+/**
+ * nbcon_legacy_emit_next_record - Print one record for an nbcon console
+ *					in legacy contexts
+ * @con:	The console to print on
+ * @handover:	Will be set to true if a printk waiter has taken over the
+ *		console_lock, in which case the caller is no longer holding
+ *		both the console_lock and the SRCU read lock. Otherwise it
+ *		is set to false.
+ * @cookie:	The cookie from the SRCU read lock.
+ *
+ * Context:	Any context which could not be migrated to another CPU.
+ * Return:	True if a record could be printed, otherwise false.
+ *
+ * This function is meant to be called by console_flush_all() to print records
+ * on nbcon consoles from legacy context (printing via console unlocking).
+ * Essentially it is the nbcon version of console_emit_next_record().
+ */
+bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+				   int cookie)
+{
+	struct nbcon_write_context wctxt = { };
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
+	bool progress = false;
+	unsigned long flags;
+
+	*handover = false;
+
+	/* Use the same procedure as console_emit_next_record(). */
+	printk_safe_enter_irqsave(flags);
+	console_lock_spinning_enable();
+	stop_critical_timings();
+
+	ctxt->console	= con;
+	ctxt->prio	= NBCON_PRIO_NORMAL;
+
+	progress = nbcon_atomic_emit_one(&wctxt);
+
+	start_critical_timings();
+	*handover = console_lock_spinning_disable_and_check(cookie);
+	printk_safe_exit_irqrestore(flags);
+
+	return progress;
+}
+
 /**
  * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
  *					write_atomic() callback
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1b14159990ba..d91771fb306e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1871,7 +1871,7 @@  static bool console_waiter;
  * there may be a waiter spinning (like a spinlock). Also it must be
  * ready to hand over the lock at the end of the section.
  */
-static void console_lock_spinning_enable(void)
+void console_lock_spinning_enable(void)
 {
 	/*
 	 * Do not use spinning in panic(). The panic CPU wants to keep the lock.
@@ -1910,7 +1910,7 @@  static void console_lock_spinning_enable(void)
  *
  * Return: 1 if the lock rights were passed, 0 otherwise.
  */
-static int console_lock_spinning_disable_and_check(int cookie)
+int console_lock_spinning_disable_and_check(int cookie)
 {
 	int waiter;
 
@@ -2948,13 +2948,22 @@  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 		cookie = console_srcu_read_lock();
 		for_each_console_srcu(con) {
 			short flags = console_srcu_read_flags(con);
+			u64 printk_seq;
 			bool progress;
 
 			if (!console_is_usable(con, flags))
 				continue;
 			any_usable = true;
 
-			progress = console_emit_next_record(con, handover, cookie);
+			if (flags & CON_NBCON) {
+				progress = nbcon_legacy_emit_next_record(con, handover, cookie);
+
+				printk_seq = nbcon_seq_read(con);
+			} else {
+				progress = console_emit_next_record(con, handover, cookie);
+
+				printk_seq = con->seq;
+			}
 
 			/*
 			 * If a handover has occurred, the SRCU read lock
@@ -2964,8 +2973,8 @@  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 				return false;
 
 			/* Track the next of the highest seq flushed. */
-			if (con->seq > *next_seq)
-				*next_seq = con->seq;
+			if (printk_seq > *next_seq)
+				*next_seq = printk_seq;
 
 			if (!progress)
 				continue;