Message-ID: <bf367mxsrilyjwg433pnoy2dqis3gw6r647zt3sztsi7zwyk4n@efzf3y4etvms>
Date: Fri, 9 Jan 2026 02:48:51 -0800
From: Breno Leitao <leitao@...ian.org>
To: John Ogness <john.ogness@...utronix.de>
Cc: pmladek@...e.com, mpdesouza@...e.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, asantostc@...il.com, efault@....de, gustavold@...il.com, 
	calvin@...nvd.org, jv@...sburgh.net, kernel-team@...a.com, 
	Simon Horman <horms@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, rostedt@...dmis.org
Subject: Re: [PATCH net-next 0/2] net: netconsole: convert to NBCON console
 infrastructure

On Thu, Jan 08, 2026 at 05:56:44PM +0100, John Ogness wrote:
> On 2026-01-08, Breno Leitao <leitao@...ian.org> wrote:
> 
> > +#ifdef CONFIG_PRINTK_EXECUTION_CTX
> > +	pid_t			pid;
> > +	int			cpu;
> > +#endif
> 
> Something like msg_pid/msg_cpu or printk_pid/printk_cpu might be better
> to make it clear we are not talking about _this_ context. This struct is
> used by code outside of the printk subsystem, which is why I think it
> needs to be more obvious what these represent.

Acknowledged.

> @Petr: Any suggestions for names (assuming this is even acceptable)?
> 
> >  };
> >  
> >  /**
> > diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
> > index eb2094e43050..42ee778b29dd 100644
> > --- a/include/linux/dev_printk.h
> > +++ b/include/linux/dev_printk.h
> > @@ -27,6 +27,10 @@ struct device;
> >  struct dev_printk_info {
> >  	char subsystem[PRINTK_INFO_SUBSYSTEM_LEN];
> >  	char device[PRINTK_INFO_DEVICE_LEN];
> > +#ifdef CONFIG_PRINTK_EXECUTION_CTX
> > +	pid_t pid;
> 
> I am not happy about this being resolved by the netconsole printer to
> get the task name. A lot can happen between now and then. But I also
> shudder at the thought of making dev_printk_info much larger. This is
> already a horrible waste of memory (which I talked about here[0]).

I encountered the same dilemma. There's also another trade-off: passing
the context from the msg to netconsole requires some string copies.

Instead of simple assignments like "pmsg->msg_pid = info->msg_pid" and
"wctxt->msg_pid = pmsg->msg_pid", we would need to use strcpy.

I'm fine with either approach — happy to defer to your preference.

> I also do not think dev_printk_info is the appropriate place to store
> this information. These new fields are not related to the dev_printk
> API. They belong in printk_info.

Acknowledged.

> > +	  This option extends struct dev_printk_info to include extra execution
> 
> It should extend printk_info instead.

Acknowledged.

> 
> > +	  context in pritnk, such as task PID and CPU number from where the
> 
>                      printk
> 
> > +	  message originated. This is useful for correlating device messages
> 
> Rather than "device messages" I suggest "printk messages".

Acknowledged.

> 
> > +	  with specific execution contexts.
> > +
> > +	  One of the main user for this config is netconsole.
> 
> Rather than saying which drivers might support this, it would probably
> be better to make it explicit. For example introducing a new config
> like:
> 
> CONFIG_CONSOLE_HAS_EXECUTION_CTX
> 
> that can only be selected by the console driver (or in your case, the
> console driver option NETCONSOLE_DYNAMIC). Then make
> PRINTK_EXECUTION_CTX depend only on CONSOLE_HAS_EXECUTION_CTX. That way
> it is only available if the console driver supports it.

Good idea. This creates a transitional layer between the context and
printk's internal configuration: consoles explicitly select
CONSOLE_HAS_EXECUTION_CTX, which in turn enables PRINTK_EXECUTION_CTX.

> > +
> >  config STACKTRACE_BUILD_ID
> >  	bool "Show build ID information in stacktraces"
> >  	depends on PRINTK
> 
> While this patch might be "good enough" to preserve the current
> CONFIG_NETCONSOLE_DYNAMIC features for NBCON, I am not happy about it:
> 
> 1. It relies on the printer context being able to determine context
> information about the printk() caller. I would prefer adding the task
> name directly to printk_info instead.

That would be straightforward to implement and would simplify things for
console users like netconsole. The trade-off is increased memory usage
in printk_info, printk_message, and nbcon_write_context, plus some
additional strscpy calls.

I don't have a strong preference here.

> 2. It adds information to printk records that only netconsole can use.
>    If we want other consoles to support this, we would need to modify
>    all the console code. I would prefer it is dynamically added to the
>    generic printing text. We could do this by extending
>    msg_print_ext_body() based on some user configuration.

This would address the problem for other consoles, but based on my
understanding of earlier discussions [0], that doesn't seem to be the
direction you guys decided.

Link: https://lore.kernel.org/lkml/20200904082438.20707-1-changki.kim@samsung.com/ [0]

>  But it would conflict with the current netconsole format.

Good point. Let me provide some additional context that may help us find
a better solution.

Netconsole's sysdata and userdata append data to printk messages in a
similar fashion to the dictionary/dev_printk_info — essentially an
extension of dev_printk_info data (though with one regrettable
inconsistency: the keys are lowercase).

Here's an example of a netconsole message with dev_printk_info plus sysdata:

  <message>
   SUBSYSTEM=net
   DEVICE=+pci:0000:00:1f.6
   cpu=42
   taskname=NetworkManager
   ...

So while the format is similar, the differences are significant enough
that moving cpu/taskname from netconsole to printk/dev_printk_info
wouldn't be trivial.

> Despite my concerns, adding the PID and CPU information is generally
> useful. So I am not against expanding printk_info. My concerns are
> more about how this information is being used by netconsole.
> 
> @Petr: I am really curious to hear your thoughts on this.

Thanks for the feedback and for helping me work through this. Let's see
what @Petr has to say, and then we can decide how to proceed.

--breno

Powered by blists - more mailing lists