Message-ID: <87ft7xazsf.fsf@jogness.linutronix.de>
Date:   Sat, 05 Sep 2020 01:33:12 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
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 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 */
>
> From my POV, if we support 3 timestamps then they must be stored
> reliably. And dict ring is out of the game.

Agreed. I am just trying to think of how to better manage the strings,
which currently are rare and optional. That is where the dict_ring
becomes interesting.

Perhaps we should use both the fixed structs with the variable
dict_ring. printk_ext_info could look like this:

struct struct printk_ext_info {
    u64 ts_boot;
    u64 ts_real;
    char *process;
    char *subsystem;
    char *device;
};

And @process, @subsystem, @device could all point to null-terminated
trings within the dict_ring. So printk.c code looks something like this:

size_t process_sz = strlen(process) + 1;
size_t subsystem_sz = strlen(subsystem) + 1;
size_t device_sz = strlen(device) + 1;
struct prb_reserved_entry e;
struct printk_record r;
char *p;

prb_rec_init_wr(&r, text_len, process_sz + subsystem_sz + device_sz);
prb_reserve(&e, prb, &r);

memcpy(r.text_buf, text, text_len);
r.info->text_len = text_len;

/* guaranteed ext data */
r.ext_info->ts_boot = time_boot();
r.ext_info->ts_real = time_real();

/* optional ext data */
if (r.dict_buf) {
    p = r.dict_buf;

    memcpy(p, process, process_sz);
    r.ext_info->process = p;
    p += process_sz;
    
    memcpy(p, subsystem, subsystem_sz);
    r.ext_info->subsystem = p;    
    p += subsystem_sz;
    
    memcpy(p, device, device_sz);
    r.ext_info->device = p;

    r.info->dict_len = process_sz + subsystem_sz + device_sz;
}

> 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.

In that case, perhaps @subsystem should be a static array in
printk_ext_info instead.

> 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 whole point of the dict_ring is that it allows for variable length
_optional_ data to be stored. If we decide there is no optional data,
then dict_ring is not needed.

> 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.

Agreed. For non-optional data (such as your timestamps), I am in full
agreement that a fixed array is the way to go. And it would only require
a couple lines of code added to the ringbuffer.

My concern is if we need to guarantee space for all possible dictionary
data of all records. I think the dict_ring can be very helpful here.

John Ogness

Powered by blists - more mailing lists