Re: [PATCH net-next 2/2] netconsole: convert to NBCON console infrastructure

[Marcos Paulo de Souza <mpdesouza@suse.com>]

On Mon, 22 Dec 2025 06:52:11 -0800 Breno Leitao <leitao@debian.org> 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
> - 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 | 95 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 58 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index dc3bd7c9b049..248b401bcaa4 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,60 @@ 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 a 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(wctxt))
> +			continue;

In this case, I believe that it should return directly? If it can't enter in the
unsafe region the output buffer is not reliable anymore, so retrying the send
the buffer to a different target isn't correct anymore. Petr, John, do you
agree?

> +
> +		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 +1939,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

Sent using hkml (https://github.com/sjp38/hackermail)