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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZvrvEF5uLAP6_4RX@google.com>
Date: Mon, 30 Sep 2024 11:33:52 -0700
From: Isaac Manjarres <isaacmanjarres@...gle.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>, surenb@...gle.com,
	kernel-team@...roid.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] printk: Improve memory usage logging during boot

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