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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zvqp5jNa7XCRfSu9@pathway.suse.cz>
Date: Mon, 30 Sep 2024 15:38:46 +0200
From: Petr Mladek <pmladek@...e.com>
To: "Isaac J. Manjarres" <isaacmanjarres@...gle.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 Wed 2024-09-25 18:12:01, Isaac J. Manjarres wrote:
> When the initial printk ring buffer size is updated, setup_log_buf()
> allocates a new ring buffer, as well as a set of meta-data structures
> for the new ring buffer. The function also emits the new size of the
> ring buffer, but not the size of the meta-data structures.
> 
> This makes it difficult to assess how changing the log buffer size
> impacts memory usage during boot.
> 
> For instance, increasing the ring buffer size from 512 KB to 1 MB
> through the command line yields an increase of 2304 KB in reserved
> memory at boot, while the only obvious change is the 512 KB
> difference in the ring buffer sizes:

Good point.

> --- 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);
}

> +
>  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;
	}

>  	new_descs_count = new_log_buf_len >> PRB_AVGBITS;
>  	if (new_descs_count == 0) {
>  		pr_err("new_log_buf_len: %lu too small\n", new_log_buf_len);
> -		return;
> +		goto out;
>  	}
>  

Best Regards,
Petr