Re: printk: Add process name information to printk() output.

From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Changki Kim <changki.kim@samsung.com>,
	sergey.senozhatsky@gmail.com, rostedt@goodmis.org,
	changbin.du@intel.com, masahiroy@kernel.org, rd.dunlap@gmail.com,
	gregkh@linuxfoundation.org, krzk@kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: printk: Add process name information to printk() output.
Date: Fri, 04 Sep 2020 15:23:19 +0206	[thread overview]
Message-ID: <87y2lp4r6o.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <20200904124530.GB20558@alley>

On 2020-09-04, Petr Mladek <pmladek@suse.com> wrote:
> I am currently playing with support for all three timestamps based
> on https://lore.kernel.org/lkml/20200814101933.574326079@linutronix.de/
>
> And I got the following idea:
>
> 1. Storing side:
>
>    Create one more ring/array for storing the optional metadata.
>    It might eventually replace dict ring, see below.
>
>    struct struct printk_ext_info {
> 	u64 ts_boot;			/* timestamp from boot clock */
> 	u64 ts_real;			/* timestamp from real clock */
> 	char process[TASK_COMM_LEN];	/* process name */
>    };
>
>    It must be in a separate array so that struct prb_desc stay stable
>    and crashdump tools do not need to be updated so often.
>
>    But the number of these structures must be the same as descriptors.
>    So it might be:
>
>    struct prb_desc_ring {
> 	unsigned int		count_bits;
> 	struct prb_desc		*descs;
> 	struct printk_ext_info  *ext_info
> 	atomic_long_t		head_id;
> 	atomic_long_t		tail_id;
>    };
>
>    One huge advantage is that these extra information would not block
>    pushing lockless printk buffer upstream.
>
>    It might be even possible to get rid of dict ring and just
>    add two more elements into struct printk_ext_info:
>
> 	  char subsystem[16];	/* for SUBSYSTEM= dict value */
> 	  char device[48];	/* for DEVICE= dict value */

You say "get rid of dict ring", but there is nothing requiring the
dict_ring to be strings. It can be binary data. The @data of the
prb_data_block struct could be a printk_ext_info struct. This would be
trivial to implement in printk.c and would not require any ringbuffer
changes. (My ringbuffer test software [0] uses binary structs for the
data.)

Using VMCOREINFO we can provide the printk_ext_info size and field
offsets for crash tools.

>    Pros:
>
> 	+ the information will always get stored

If the dict_ring is "_DESCS_COUNT() * sizeof(struct printk_ext_info)"
then it would also always get stored. Although this does seem like a bit
of a waste of space in order to cover the worst case scenario of all
records using all fields.

John Ogness

[0] https://github.com/Linutronix/prb-test.git