[<prev] [next>] [<thread-prev] [thread-next>] [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
Powered by blists - more mailing lists