Re: [PATCH v1] printk: Improve memory usage logging during boot

[Isaac Manjarres <isaacmanjarres@google.com>]

On Mon, Sep 30, 2024 at 03:38:46PM +0200, Petr Mladek wrote:
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1156,6 +1156,17 @@ static unsigned int __init add_to_rb(struct printk_ringbuffer *rb,
> >  
> >  static char setup_text_buf[PRINTKRB_RECORD_MAX] __initdata;
> >  
> > +static void print_log_buf_usage_stats(void)
> > +{
> > +	unsigned int descs_count = log_buf_len >> PRB_AVGBITS;
> > +	size_t descs_size = descs_count * sizeof(struct prb_desc);
> > +	size_t infos_size = descs_count * sizeof(struct printk_info);
> > +
> > +	pr_info("log_buf_len: %u bytes\n", log_buf_len);
> > +	pr_info("prb_descs size: %zu bytes\n", descs_size);
> > +	pr_info("printk_infos size: %zu bytes\n", infos_size);
> > +}
> 
> I would make the information more user friendly. Also a single line
> might be enough. Something like:
> 
> static void print_log_buf_usage_stats(void)
> {
> 	unsigned int descs_count = log_buf_len >> PRB_AVGBITS;
> 	size_t meta_data_size;
> 
> 	meta_data_size = descs_count *
> 		(sizeof(struct prb_desc) + sizeof(struct printk_info));
> 
> 	pr_info("log buffer data + meta data: %u + %zu = %zu bytes\n",
> 		log_buf_len, meta_data_size, log_buf_len + meta_data_size);
> }
Thanks for suggesting this! I'll do this for the 2nd version of the
patch.

> > +
> >  void __init setup_log_buf(int early)
> >  {
> >  	struct printk_info *new_infos;
> > @@ -1186,19 +1197,19 @@ void __init setup_log_buf(int early)
> >  		log_buf_add_cpu();
> >  
> >  	if (!new_log_buf_len)
> > -		return;
> > +		goto out;
> 
> The same information is printed twice when the default buffer is used.
> We should do something like:
> 
> 	if (!new_log_buf_len) {
> 		if (early)
> 			goto out;
> 		return;
> 	}
> 
Thank you for pointing this out. I'll do something very similar to this
in the 2nd version of the patch, but I'll use "!early" instead. The
rationale is that if I use just use "early", then the memory usage
stats don't get emitted at all on my machine (arm64) when it uses the
default buffer, because setup_log_buf() is called only once with
early == 0.

Using !early in the check there should fix that, and also emit the
memory stats only once on machines that invoke setup_log_buf()
multiple times and end up using the default buffer.

Thanks,
Isaac