Date:   Fri, 4 Sep 2020 17:13:36 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Changki Kim <changki.kim@...sung.com>,
        sergey.senozhatsky@...il.com, rostedt@...dmis.org,
        changbin.du@...el.com, masahiroy@...nel.org, rd.dunlap@...il.com,
        gregkh@...uxfoundation.org, krzk@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: printk: Add process name information to printk() output.

On Fri 2020-09-04 15:23:19, John Ogness wrote:
> On 2020-09-04, Petr Mladek <pmladek@...e.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.

The opposite is that the extended metadata might be missing for some
lines. This might cause quite some confusion.

For example, I can't find any reasonable way how to produce missing
timestamps.

>From my POV, if we support 3 timestamps then they must be stored
reliably. And dict ring is out of the game.


And I am not comfortable even with the current dictionary handling.
I already wrote this somewhere. The following command is supposed
to show all kernel messages printed by "pci" subsystem:

	$> journalctl _KERNEL_SUBSYSTEM=pci

It will be incomplete when the dictionary metadata were not saved.

You might argue that the problem already exists when the entire
message gets lost. But this situation can be detected by missing
sequence numbers and journalctl even add messages about it.
But the user currently is not informed about missing
dictionary.


Regarding the waste of space. The dict ring currently has the same
size as the text ring. It is likely a waste of space as well. Any tuning
is complicated because it depends on the use case.

The advantage of the fixed @ext_info[] array is that everything is
clear, simple, and predictable (taken space and name length limits).
We could easily tell users what they will get for a given cost.

Best Regards,
Petr

Powered by blists - more mailing lists