Message-ID: <5hgyof3yowdw3v76ygz2oxkzv7vpz5kp62nx36gynmr646yrjs@ag4mvynlin4k>
Date: Wed, 17 Sep 2025 07:44:34 -0700
From: Breno Leitao <leitao@...ian.org>
To: John Ogness <john.ogness@...utronix.de>
Cc: Petr Mladek <pmladek@...e.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt <rostedt@...dmis.org>, 
	Mike Galbraith <efault@....de>, linux-kernel@...r.kernel.org, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 0/1] Allow unsafe ->write_atomic() for panic

On Fri, Sep 12, 2025 at 02:24:51PM +0206, John Ogness wrote:
> Hi,
> 
> An effort is underway to update netconsole to support nbcon [0].
> Currently it is not known exactly how a safe ->write_atomic()
> callback for netconsole could be implemented. However, without
> ->write_atomic() implemented, there is guaranteed to be _no_
> panic output.
> 
> We decided to allow unsafe ->write_atomic() implementations to
> be used only as a last resort at the end of panic. This should
> allow netconsole to be updated to support nbcon and still
> provide panic output in "typical panic" situations.
> 
> I considered extending the feature to also allow such consoles
> to print using their ->write_thread() callback whenever it is
> known to be a sleepable context. However, this started to get
> quite complex. We can revisit this extended feature if it is
> determined that it is necessary.

Thanks for this patch! I've successfully adapted my PoC netconsole
implementation to work with NBCON using your modifications, and the
initial results are encouraging. With your patch and a corresponding
->write_atomic callback, I'm receiving the complete set of messages that
were previously available through the regular console interface.

Upon further consideration, it's worth noting that not all network
drivers rely on irq-unsafe locks. In practice, only a subset of drivers
use them, while most network drivers I'm familiar with maintain IRQ-safe
TX paths.

If we could determine the IRQ safety characteristics (IRQ-safe vs
IRQ-unsafe TX) during netconsole registration, this would enable more
optimized behavior: netconsole could register as CON_NBCON_ATOMIC_UNSAFE
only when the underlying network adapter uses IRQ-unsafe locks. For
adapters with IRQ-safe implementations, netconsole could safely utilize
the ->write_atomic path without restrictions.

FWIW, I am attaching the patch I used to test netconsole on top of your
changes. Keep in mind this is is calling TX patch with IRQ disabled
given the list is depending on IRQ-safe lock.

This will be fixed once I do the following:

1) move target_list to use RCU
2) Untangling netconsole from netpoll [1]
3) They are depending on a conflicting netpoll fix [2]

Link: https://lore.kernel.org/all/willemdebruijn.kernel.a0f67bb6112a@gmail.com/ [1]
Link: https://lore.kernel.org/all/20250917-netconsole_torture-v4-1-0a5b3b8f81ce@debian.org/ [2]

commit e9f4a292a49ae6d3da29f1dca39754180d2608d7
Author: Breno Leitao <leitao@...ian.org>
Date:   Tue Aug 19 04:14:58 2025 -0700

    netconsole: Add support for nbcon
    
    Add support for running netconsole using the new non‑blocking console
    (nbcon) infrastructure.
    
    The nbcon framework improves console handling by avoiding the global
    console lock and enabling asynchronous, non‑blocking writes from
    multiple contexts.
    
    With this option enabled, netconsole can operate as a fully
    asynchronous, lockless nbcon backend, not depending on console lock
    anymore.
    
    This support is marked experimental for now until it receives wider
    testing.
    
    This depends on the CON_NBCON_ATOMIC_UNSAFE nbcon flag, and
    ->write_atomic is unsafe and only called with NBCON_PRIO_PANIC priority.
    
    Signed-off-by: Breno Leitao <leitao@...ian.org>

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index b29628d46be9b..ec9a430aa160e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -382,6 +382,16 @@ config NETCONSOLE_PREPEND_RELEASE
 	  message.  See <file:Documentation/networking/netconsole.rst> for
 	  details.
 
+config NETCONSOLE_NBCON
+	bool "Enable non blocking netconsole (EXPERIMENTAL)"
+	depends on NETCONSOLE
+	default n
+	help
+	  Move netconsole to use non-blocking console (nbcons).  Non-blocking
+	  console (nbcon) is a new console infrastructure introduced to improve
+	  console handling by avoiding the global console lock (Big Kernel
+	  Lock) and enabling non-blocking, asynchronous writes to the console.
+
 config NETPOLL
 	def_bool NETCONSOLE
 
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index e3722de08ea9f..ac2f0ace45d6e 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1692,6 +1692,97 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 				   extradata_len);
 }
 
+static void do_write_msg(struct netconsole_target *nt, const char *msg,
+			 unsigned int len)
+{
+	const char *tmp;
+	int frag, left;
+
+	/*
+	 * We nest this inside the for-each-target loop above
+	 * so that we're able to get as much logging out to
+	 * at least one target if we die inside here, instead
+	 * of unnecessarily keeping all targets in lock-step.
+	 */
+	tmp = msg;
+	for (left = len; left;) {
+		frag = min(left, MAX_PRINT_CHUNK);
+		send_udp(nt, tmp, frag);
+		tmp += frag;
+		left -= frag;
+	}
+}
+
+#ifdef CONFIG_NETCONSOLE_NBCON
+static void netcon_write_ext_atomic(struct console *con,
+				    struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			send_ext_msg_udp(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+static void netcon_write_ext_thread(struct console *con,
+				    struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			send_ext_msg_udp(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+/* regular write call backs */
+static void netcon_write_thread(struct console *con,
+				struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			do_write_msg(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+static void netcon_write_atomic(struct console *con,
+				struct nbcon_write_context *wctxt)
+{
+	struct netconsole_target *nt;
+
+	list_for_each_entry(nt, &target_list, list)
+		if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) {
+			if (!nbcon_enter_unsafe(wctxt))
+				continue;
+			do_write_msg(nt, wctxt->outbuf, wctxt->len);
+			nbcon_exit_unsafe(wctxt);
+		}
+}
+
+/* locks call backs */
+static void netconsole_device_lock(struct console *con, unsigned long *flags)
+{
+	/* protects all the targets at the same time */
+	spin_lock_irqsave(&target_list_lock, *flags);
+}
+
+static void netconsole_device_unlock(struct console *con, unsigned long flags)
+{
+	spin_unlock_irqrestore(&target_list_lock, flags);
+}
+#else
 static void write_ext_msg(struct console *con, const char *msg,
 			  unsigned int len)
 {
@@ -1710,10 +1801,8 @@ static void write_ext_msg(struct console *con, const char *msg,
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
-	int frag, left;
 	unsigned long flags;
 	struct netconsole_target *nt;
-	const char *tmp;
 
 	if (oops_only && !oops_in_progress)
 		return;
@@ -1723,24 +1812,13 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
-		if (!nt->extended && nt->enabled && netif_running(nt->np.dev)) {
-			/*
-			 * We nest this inside the for-each-target loop above
-			 * so that we're able to get as much logging out to
-			 * at least one target if we die inside here, instead
-			 * of unnecessarily keeping all targets in lock-step.
-			 */
-			tmp = msg;
-			for (left = len; left;) {
-				frag = min(left, MAX_PRINT_CHUNK);
-				send_udp(nt, tmp, frag);
-				tmp += frag;
-				left -= frag;
-			}
-		}
+		if (!nt->extended && nt->enabled && netif_running(nt->np.dev))
+			do_write_msg(nt, msg, len);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
+#endif
+
 
 static int netpoll_parse_ip_addr(const char *str, union inet_addr *addr)
 {
@@ -1919,14 +1997,30 @@ static void free_param_target(struct netconsole_target *nt)
 
 static struct console netconsole_ext = {
 	.name	= "netcon_ext",
+#ifdef CONFIG_NETCONSOLE_NBCON
+	.flags	= CON_ENABLED | CON_EXTENDED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
+	.write_thread = netcon_write_ext_thread,
+	.write_atomic = netcon_write_ext_atomic,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
+#else
 	.flags	= CON_ENABLED | CON_EXTENDED,
 	.write	= write_ext_msg,
+#endif
 };
 
 static struct console netconsole = {
 	.name	= "netcon",
+#ifdef CONFIG_NETCONSOLE_NBCON
+	.flags	= CON_ENABLED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
+	.write_thread = netcon_write_thread,
+	.write_atomic = netcon_write_atomic,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
+#else
 	.flags	= CON_ENABLED,
 	.write	= write_msg,
+#endif
 };
 
 static int __init init_netconsole(void)

Powered by blists - more mailing lists