[PATCH net-next v2 3/5] netconsole: convert to NBCON console infrastructure

Breno Leitao posted 5 patches 2 weeks, 5 days ago
There is a newer version of this series
[PATCH net-next v2 3/5] netconsole: convert to NBCON console infrastructure
Posted by Breno Leitao 2 weeks, 5 days ago
Convert netconsole from the legacy console API to the NBCON framework.
NBCON provides threaded printing which unblocks printk()s and flushes in
a thread, decoupling network TX from printk() when netconsole is
in use.

Since netconsole relies on the network stack which cannot safely operate
from all atomic contexts, mark both consoles with
CON_NBCON_ATOMIC_UNSAFE. (See discussion in [1])

CON_NBCON_ATOMIC_UNSAFE restricts write_atomic() usage to emergency
scenarios (panic) where regular messages are sent in threaded mode.

Implementation changes:
- Unify write_ext_msg() and write_msg() into netconsole_write()
- Add device_lock/device_unlock callbacks to manage target_list_lock
- Use nbcon_enter_unsafe()/nbcon_exit_unsafe() around network
  operations.
  - If nbcon_enter_unsafe() fails, just return given netconsole lost
    the ownership of the console.
- Set write_thread and write_atomic callbacks (both use same function)

Link: https://lore.kernel.org/all/b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt/ [1]
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 97 ++++++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 37 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index dc3bd7c9b0498..c5d7e97fe2a78 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1709,22 +1709,6 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 				   sysdata_len);
 }
 
-static void write_ext_msg(struct console *con, const char *msg,
-			  unsigned int len)
-{
-	struct netconsole_target *nt;
-	unsigned long flags;
-
-	if ((oops_only && !oops_in_progress) || list_empty(&target_list))
-		return;
-
-	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))
-			send_ext_msg_udp(nt, msg, len);
-	spin_unlock_irqrestore(&target_list_lock, flags);
-}
-
 static void send_msg_udp(struct netconsole_target *nt, const char *msg,
 			 unsigned int len)
 {
@@ -1739,29 +1723,62 @@ static void send_msg_udp(struct netconsole_target *nt, const char *msg,
 	}
 }
 
-static void write_msg(struct console *con, const char *msg, unsigned int len)
+/**
+ * netconsole_write - Generic function to send a msg to all targets
+ * @wctxt: nbcon write context
+ * @extended: "true" for extended console mode
+ *
+ * Given an nbcon write context, send the message to the netconsole targets
+ */
+static void netconsole_write(struct nbcon_write_context *wctxt, bool extended)
 {
-	unsigned long flags;
 	struct netconsole_target *nt;
 
 	if (oops_only && !oops_in_progress)
 		return;
-	/* Avoid taking lock and disabling interrupts unnecessarily */
-	if (list_empty(&target_list))
-		return;
 
-	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.
-			 */
-			send_msg_udp(nt, msg, len);
-		}
+		if (nt->extended != extended || !nt->enabled ||
+		    !netif_running(nt->np.dev))
+			continue;
+
+		/* If nbcon_enter_unsafe() fails, just return given netconsole
+		 * lost the ownership, and iterating over the targets will not
+		 * be able to re-acquire.
+		 */
+		if (!nbcon_enter_unsafe(wctxt))
+			return;
+
+		if (extended)
+			send_ext_msg_udp(nt, wctxt->outbuf, wctxt->len);
+		else
+			send_msg_udp(nt, wctxt->outbuf, wctxt->len);
+
+		nbcon_exit_unsafe(wctxt);
 	}
+}
+
+static void netconsole_write_ext(struct console *con __always_unused,
+				 struct nbcon_write_context *wctxt)
+{
+	netconsole_write(wctxt, true);
+}
+
+static void netconsole_write_basic(struct console *con __always_unused,
+				   struct nbcon_write_context *wctxt)
+{
+	netconsole_write(wctxt, false);
+}
+
+static void netconsole_device_lock(struct console *con __always_unused,
+				   unsigned long *flags)
+{
+	spin_lock_irqsave(&target_list_lock, *flags);
+}
+
+static void netconsole_device_unlock(struct console *con __always_unused,
+				     unsigned long flags)
+{
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
 
@@ -1924,15 +1941,21 @@ static void free_param_target(struct netconsole_target *nt)
 }
 
 static struct console netconsole_ext = {
-	.name	= "netcon_ext",
-	.flags	= CON_ENABLED | CON_EXTENDED,
-	.write	= write_ext_msg,
+	.name = "netcon_ext",
+	.flags = CON_ENABLED | CON_EXTENDED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
+	.write_thread = netconsole_write_ext,
+	.write_atomic = netconsole_write_ext,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
 };
 
 static struct console netconsole = {
-	.name	= "netcon",
-	.flags	= CON_ENABLED,
-	.write	= write_msg,
+	.name = "netcon",
+	.flags = CON_ENABLED | CON_NBCON | CON_NBCON_ATOMIC_UNSAFE,
+	.write_thread = netconsole_write_basic,
+	.write_atomic = netconsole_write_basic,
+	.device_lock = netconsole_device_lock,
+	.device_unlock = netconsole_device_unlock,
 };
 
 static int __init init_netconsole(void)

-- 
2.47.3
Re: [PATCH net-next v2 3/5] netconsole: convert to NBCON console infrastructure
Posted by Simon Horman 2 weeks, 3 days ago
On Tue, Jan 20, 2026 at 08:23:49AM -0800, Breno Leitao wrote:
> Convert netconsole from the legacy console API to the NBCON framework.
> NBCON provides threaded printing which unblocks printk()s and flushes in
> a thread, decoupling network TX from printk() when netconsole is
> in use.
> 
> Since netconsole relies on the network stack which cannot safely operate
> from all atomic contexts, mark both consoles with
> CON_NBCON_ATOMIC_UNSAFE. (See discussion in [1])
> 
> CON_NBCON_ATOMIC_UNSAFE restricts write_atomic() usage to emergency
> scenarios (panic) where regular messages are sent in threaded mode.
> 
> Implementation changes:
> - Unify write_ext_msg() and write_msg() into netconsole_write()
> - Add device_lock/device_unlock callbacks to manage target_list_lock
> - Use nbcon_enter_unsafe()/nbcon_exit_unsafe() around network
>   operations.
>   - If nbcon_enter_unsafe() fails, just return given netconsole lost
>     the ownership of the console.
> - Set write_thread and write_atomic callbacks (both use same function)
> 
> Link: https://lore.kernel.org/all/b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt/ [1]
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 97 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 60 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c

...

> +static void netconsole_device_lock(struct console *con __always_unused,
> +				   unsigned long *flags)
> +{
> +	spin_lock_irqsave(&target_list_lock, *flags);
> +}
> +
> +static void netconsole_device_unlock(struct console *con __always_unused,
> +				     unsigned long flags)
> +{
>  	spin_unlock_irqrestore(&target_list_lock, flags);
>  }
>  

Hi Breno,

I'm wondering if we could consider the following annotations,
as "suggested" by Sparse[1].

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index c5d7e97fe2a7..79e39a6c5343 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1772,12 +1772,14 @@ static void netconsole_write_basic(struct console *con __always_unused,
 
 static void netconsole_device_lock(struct console *con __always_unused,
 				   unsigned long *flags)
+__acquires(&target_list_lock)
 {
 	spin_lock_irqsave(&target_list_lock, *flags);
 }
 
 static void netconsole_device_unlock(struct console *con __always_unused,
 				     unsigned long flags)
+__releases(&target_list_lock)
 {
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }

[1] This particular commit of Sparse, from Al Viro's tree:
    https://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/commit/?id=2634e39bf02697a18fece057208150362c985992
    Which addresses this mess:
    https://lore.kernel.org/all/bf5b9a62-a120-421e-908d-1404c42e0b60@kernel.org/
Re: [PATCH net-next v2 3/5] netconsole: convert to NBCON console infrastructure
Posted by Breno Leitao 2 weeks, 2 days ago
Hello Simon!

On Thu, Jan 22, 2026 at 09:09:34PM +0000, Simon Horman wrote:
> On Tue, Jan 20, 2026 at 08:23:49AM -0800, Breno Leitao wrote:
> > Convert netconsole from the legacy console API to the NBCON framework.
> > NBCON provides threaded printing which unblocks printk()s and flushes in
> > a thread, decoupling network TX from printk() when netconsole is
> > in use.
> > 
> > Since netconsole relies on the network stack which cannot safely operate
> > from all atomic contexts, mark both consoles with
> > CON_NBCON_ATOMIC_UNSAFE. (See discussion in [1])
> > 
> > CON_NBCON_ATOMIC_UNSAFE restricts write_atomic() usage to emergency
> > scenarios (panic) where regular messages are sent in threaded mode.
> > 
> > Implementation changes:
> > - Unify write_ext_msg() and write_msg() into netconsole_write()
> > - Add device_lock/device_unlock callbacks to manage target_list_lock
> > - Use nbcon_enter_unsafe()/nbcon_exit_unsafe() around network
> >   operations.
> >   - If nbcon_enter_unsafe() fails, just return given netconsole lost
> >     the ownership of the console.
> > - Set write_thread and write_atomic callbacks (both use same function)
> > 
> > Link: https://lore.kernel.org/all/b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt/ [1]
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/net/netconsole.c | 97 ++++++++++++++++++++++++++++++------------------
> >  1 file changed, 60 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> 
> ...
> 
> > +static void netconsole_device_lock(struct console *con __always_unused,
> > +				   unsigned long *flags)
> > +{
> > +	spin_lock_irqsave(&target_list_lock, *flags);
> > +}
> > +
> > +static void netconsole_device_unlock(struct console *con __always_unused,
> > +				     unsigned long flags)
> > +{
> >  	spin_unlock_irqrestore(&target_list_lock, flags);
> >  }
> >  
> 
> Hi Breno,
> 
> I'm wondering if we could consider the following annotations,
> as "suggested" by Sparse[1].

This is great. I hadn't realized that Al Viro's sparse tree includes this
additional check, which is really useful.

Are you using Al Viro's branch rather than the sparse mainline?

(I'm asking to see if I should also follow master and do the same, in
true padawan fashion)
Re: [PATCH net-next v2 3/5] netconsole: convert to NBCON console infrastructure
Posted by Simon Horman 2 weeks, 2 days ago
On Fri, Jan 23, 2026 at 02:48:47AM -0800, Breno Leitao wrote:
> Hello Simon!
> 
> On Thu, Jan 22, 2026 at 09:09:34PM +0000, Simon Horman wrote:
> > On Tue, Jan 20, 2026 at 08:23:49AM -0800, Breno Leitao wrote:
> > > Convert netconsole from the legacy console API to the NBCON framework.
> > > NBCON provides threaded printing which unblocks printk()s and flushes in
> > > a thread, decoupling network TX from printk() when netconsole is
> > > in use.
> > > 
> > > Since netconsole relies on the network stack which cannot safely operate
> > > from all atomic contexts, mark both consoles with
> > > CON_NBCON_ATOMIC_UNSAFE. (See discussion in [1])
> > > 
> > > CON_NBCON_ATOMIC_UNSAFE restricts write_atomic() usage to emergency
> > > scenarios (panic) where regular messages are sent in threaded mode.
> > > 
> > > Implementation changes:
> > > - Unify write_ext_msg() and write_msg() into netconsole_write()
> > > - Add device_lock/device_unlock callbacks to manage target_list_lock
> > > - Use nbcon_enter_unsafe()/nbcon_exit_unsafe() around network
> > >   operations.
> > >   - If nbcon_enter_unsafe() fails, just return given netconsole lost
> > >     the ownership of the console.
> > > - Set write_thread and write_atomic callbacks (both use same function)
> > > 
> > > Link: https://lore.kernel.org/all/b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt/ [1]
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  drivers/net/netconsole.c | 97 ++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 60 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > 
> > ...
> > 
> > > +static void netconsole_device_lock(struct console *con __always_unused,
> > > +				   unsigned long *flags)
> > > +{
> > > +	spin_lock_irqsave(&target_list_lock, *flags);
> > > +}
> > > +
> > > +static void netconsole_device_unlock(struct console *con __always_unused,
> > > +				     unsigned long flags)
> > > +{
> > >  	spin_unlock_irqrestore(&target_list_lock, flags);
> > >  }
> > >  
> > 
> > Hi Breno,
> > 
> > I'm wondering if we could consider the following annotations,
> > as "suggested" by Sparse[1].
> 
> This is great. I hadn't realized that Al Viro's sparse tree includes this
> additional check, which is really useful.
> 
> Are you using Al Viro's branch rather than the sparse mainline?
> 
> (I'm asking to see if I should also follow master and do the same, in
> true padawan fashion)

Well, I think it would be best if mainline was fixed.
(Although I am yet to do anything towards making that happen.)

But to answer your question, yes, I am using Al's tree.
Since a few days ago when I tracked down that it allows
Sparse to once work significantly more robustly on the
current Kernel tree.