On 2020-09-04, Petr Mladek <pmla...@suse.com> wrote: >>> I am currently playing with support for all three timestamps based >>> on https://lore.kernel.org/lkml/20200814101933.574326...@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