Message-ID: <aWUCc73o4UeYtz-z@pathway.suse.cz>
Date: Mon, 12 Jan 2026 15:17:23 +0100
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Breno Leitao <leitao@...ian.org>, 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 Fri 2026-01-09 16:19:31, John Ogness wrote:
> On 2026-01-09, Petr Mladek <pmladek@...e.com> wrote:
> > If we were adding this info, I would add it for all users, definitely.
> > We are going to touch the internal printk ringbuffer format anyway.
> 
> Which could mean we are going to need a round of updating crash/dump
> tools. So we should choose wisely.

Exactly :-)

> > Now, the main question is how far we want to go.
> >
> > I see the following possibilities:
> >
> > A) caller_id -> pid + cpu + atomic/task context bit
> > ===================================================
> >
> > Replace the current .caller_id and always save both PID and CPU
> > number. Something like:
> >
> > diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
> > index 4ef81349d9fb..3c7635ada6dd 100644
> > --- a/kernel/printk/printk_ringbuffer.h
> > +++ b/kernel/printk/printk_ringbuffer.h
> > @@ -9,6 +9,12 @@
> >  #include <linux/stddef.h>
> >  #include <linux/types.h>
> >  
> > +
> > +struct printk_caller {
> > +	pid_t	pid;	/* caller pid */
> > +	int	cpu;	/* caller CPU number */
> > +};
> > +
> >  /*
> >   * Meta information about each stored message.
> >   *
> > @@ -22,8 +29,8 @@ struct printk_info {
> >  	u8	facility;	/* syslog facility */
> >  	u8	flags:5;	/* internal record flags */
> >  	u8	level:3;	/* syslog level */
> > -	u32	caller_id;	/* thread id or processor id */
> >  
> > +	struct printk_caller caller;
> >  	struct dev_printk_info	dev_info;
> >  };
> >
> > Plus the task/interrupt bit might be stored either as the highest bit
> > in .pid or by adding LOG_CALLER_IN_TASK bit into "enum printk_info_flags".
> 
> The thing I find attractive about this solution is that it could be done
> such that crash/dump tools must not be changed. We could leave the
> semantics for @caller_id as is and simply add @cpu:
>
> --- a/kernel/printk/printk_ringbuffer.h
> +++ b/kernel/printk/printk_ringbuffer.h
> @@ -23,6 +23,7 @@ struct printk_info {
>  	u8	flags:5;	/* internal record flags */
>  	u8	level:3;	/* syslog level */
>  	u32	caller_id;	/* thread id or processor id */
> +	u32	cpu;	/* processor id */
>  
>  	struct dev_printk_info	dev_info;
>  };
> 
> After all, if the caller is not in_task() then the new @pid would be
> meaningless anyway.

Or we could use:

	u32	caller_id2;	/* processor id or thread id,

, where .caller_id2 is a complement to caller_id which is backward
compatible. The highest bit in @caller_id would define what
is stored where.

We could create helpers to encode/decode it:

	set_printk_info_cpu(struct printk_info *info, int cpu);
	get_printk_info_cpu(struct printk_info *info);
	...

It is a bit ugly. But storing CPU twice is kind of ugly as well.
And it will provide all information for netconsole.


> If we are willing to accept printer-resolution of task names, then this
> simple addition would be good enough for netconsole, while not requiring
> any crash/dump tool updates. This would buy us time to think more
> seriously about a significant overhaul.

I am just a bit afraid that this might stay quite a long time
until anyone gets motivation and time to clean it.


> > B) caller_id -> pid + cpu + contex
> > ==================================
> >
> > Same as above but the caller context info is stored separately and
> > might allow to distinguish more types. Something like:
> >
> > diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
> > index 4ef81349d9fb..44aa60a3f84c 100644
> > --- a/kernel/printk/printk_ringbuffer.h
> > +++ b/kernel/printk/printk_ringbuffer.h
> > @@ -9,6 +9,19 @@
> >  #include <linux/stddef.h>
> >  #include <linux/types.h>
> >  
> > +#define PRINTK_CALLER_CTXT_PREEMPT_BIT		1
> > +#define PRINTK_CALLER_CTXT_SOFTIRQ_BIT		2
> > +#define PRINTK_CALLER_CTXT_HARDIRQ_BIT		3
> > +#define PRINTK_CALLER_CTXT_NMI			4
> > +/* This is not supported on all platforms, see irqs_disabled() */
> > +#define PRINTK_CALLER_CTXT_IRQS_DISABLED	5
> > +
> > +struct printk_caller {
> > +	u8	ctxt;	/* caller context: task, irq, ... */
> > +	pid_t	pid;	/* caller pid */
> > +	int	cpu;	/* caller CPU number */
> > +};
> > +
> >  /*
> >   * Meta information about each stored message.
> >   *
> > @@ -22,8 +35,8 @@ struct printk_info {
> >  	u8	facility;	/* syslog facility */
> >  	u8	flags:5;	/* internal record flags */
> >  	u8	level:3;	/* syslog level */
> > -	u32	caller_id;	/* thread id or processor id */
> >  
> > +	struct printk_caller caller;
> >  	struct dev_printk_info	dev_info;
> >  };
> 
> Just as with A, here we could also preserve @caller_id semantics
> (instead of introducing @pid) to avoid crash/dump tool updates.
> 
> A new @ctxt field is only useful if it can be seen. I am not sure how
> you plan on showing this. By extending the prefix like caller_id does?

I am afraid that we would need to come up with another format.
I wanted to get some inspiration from lockdep. And check other
subystems which might already show this (backtrace, ftrace, ...)

> Would it be a new kernel config or just use CONFIG_PRINTK_CALLER? Either
> way, we are talking about extending visible log data, which is something
> that needs a discussion on its own.

Sure. We should think twice about it.

> > C) caller_id -> pid + cpu + comm + atomic/task context bit
> > ==========================================================
> >
> > Similar to A and B but add also
> >
> > 	char comm[TASK_COMM_LEN];
> >
> > into struct printk_caller.
> 
> Just as with A and B, we could preserve @caller_id semantics.
> 
> This is the simplest solution since the printer would not need to
> perform any resolution at all and it would be 100% accurate (when
> in_task).
> 
> > D) caller_id -> pid + cpu + comm + atomic/task context bit
> >    and move printk_caller + printk_dev_info into text buffer
> >    as suggested as mentioned at
> >    https://lore.kernel.org/lkml/84y10vz7ty.fsf@jogness.linutronix.de
> > ====================================================================
> >
> >    There are many possibilities how to do it. And it involves two
> >    problems:
> >
> >       a) how to store the data into data buffer
> >       b) how to integrate this in the ring buffer API
> >
> >     I thought about several approaches and realized that it still
> >     would make sense to keep:
> >
> >        + binary data (pid, cpu, ctxt) in desc ring
> >        + text data (comm, subsystem, device) in text_data buffer
> >
> >     Something like:
> >
> > diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
> > index 4ef81349d9fb..41232bf2919d 100644
> > --- a/kernel/printk/printk_ringbuffer.h
> > +++ b/kernel/printk/printk_ringbuffer.h
> > @@ -9,6 +9,27 @@
> >  #include <linux/stddef.h>
> >  #include <linux/types.h>
> >  
> > +#define PRINTK_CALLER_CTXT_PREEMPT_BIT		0
> > +#define PRINTK_CALLER_CTXT_SOFTIRQ_BIT		1
> > +#define PRINTK_CALLER_CTXT_HARDIRQ_BIT		2
> > +#define PRINTK_CALLER_CTXT_NMI			3
> > +/* This is not supported on all platforms, see irqs_disabled() */
> > +#define PRINTK_CALLER_CTXT_IRQS_DISABLED	4
> > +
> > +struct printk_caller {
> > +	u8	ctxt;	/* caller context: task, irq, ... */
> > +	pid_t	pid;	/* caller pid */
> > +	int	cpu;	/* caller CPU number */
> > +};
> > +
> > +/*
> > + * Describe which text information about the caller
> > + * is stored in text_data_ring
> > + */
> > +#define PRINTK_CALLER_BITS_COMM_BIT		0
> > +#define PRINTK_CALLER_BITS_SUBSYSTEM_BIT	1
> > +#define PRINTK_CALLER_BITS_DEVICE_BIT		2
> > +
> >  /*
> >   * Meta information about each stored message.
> >   *
> > @@ -22,9 +43,8 @@ struct printk_info {
> >  	u8	facility;	/* syslog facility */
> >  	u8	flags:5;	/* internal record flags */
> >  	u8	level:3;	/* syslog level */
> > -	u32	caller_id;	/* thread id or processor id */
> > -
> > -	struct dev_printk_info	dev_info;
> > +	u8	caller_bits_size; /* size of the caller info in data buffer */
> > +	u8	caller_bits;	/* bits describing caller in data buffer */
> >  };
> >  
> >  /*
> >
> > The text would be stored in the test_data_ring in the following
> > format:
> >
> > 	[<comm>\0][<subsystem>\0][<device\0]<text>\0
> 
> I would prefer:
> 
> <text>\0[<subsystem>\0][<device>\0][<comm>\0]

We could discuss this when there is some code. IMHO, we should keep
the ordering similar to the console output. It might help when
when people see plain memory dump of the data_ring buffer.

> > My opinion:
> > ===========
> >
> > I personally think that the approach B) is the best compromise for now
> > because:
> 
> My ordered preferences for right now would be:
> 
> 1. keeping @caller_id semantics + adding @cpu + adding @comm (similar to
> your C)

I am fine with this if the crash tools are really able to handle it
out of box.

> 2. keeping @caller_id semantics + adding @cpu (similar to your A)
> 
> These additions would be purely to support netconsole and the additions
> would be under CONFIG_NETCONSOLE_DYNAMIC. No crash/dump tools should be
> changed due to this.
>
> For the long term I would like to move all strings into the
> text_data_ring. We could use that opportunity to add context bits, time
> stamps, etc. and then appropriately update the major crash/dump tools.

That would be great.

Best Regards,
Petr

Powered by blists - more mailing lists