> 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
To be honest, This patch is made based on kernel 5.4. So I haven't fully figured out the printk ring buffer yet. When I see the code, dict_buf didn't print out into prefix. Right? I want to print out the information into prefix. When I developed the kernel in Android, we could modify prefix of kernel log format. Because we should be able to obtain a lot of information with just a kernel log. But now, we can't modify the prefix due to GKI. So We need a printk where we can freely modify the prefix. The prefix doesn't need to be saved to log_buf. It only needs to be printed out. It would be great if printk.format is possible. printk.format=ts,cpu,comm,pid,in_atomic Changki Kim.
