Re: [RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Petr Mladek <pmladek@suse.com>]

On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
> to kmemdump.
> This will allow kmemdump to be able to dump specific log buffer areas on
> demand.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>  
> +void kmsg_kmemdump_register(void)
> +{
> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));

This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
It should be enough to register the memory with the structure.

> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
> +			  sizeof(_printk_rb_static_descs));
> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
> +			  sizeof(_printk_rb_static_infos));

Also this looks wrong. These are static buffers which are used during
early boot. They might later be replaced by dynamically allocated
buffers when a bigger buffer is requested by "log_buf_len" command
line parameter.

I think that we need to register the memory of the structure
and 3 more buffers. See how the bigger buffer is allocated in
setup_log_buf().

I would expect something like:

	unsigned int descs_count;
	unsigned long data_size;

	descs_count = 2 << prb->desc_ring.count_bits;
	data_size = 2 << prb->data_ring.size_bits;

	kmemdump_register("prb", (void *)prb, sizeof(*prb));
	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
			  descs_count * sizeof(struct prb_desc));
	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
			  descs_count * sizeof(struct printk_info));
	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);


But I wonder if this is enough. The current crash dump code also needs
to export the format of the used structures, see
log_buf_vmcoreinfo_setup().

Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

> +}
> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
> +

Best Regards,
Petr