[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y5GuYvj6VRnhN0+H@alley>
Date: Thu, 8 Dec 2022 10:29:06 +0100
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v2 7/7] printk: Handle dropped message smarter
On Wed 2022-12-07 18:04:56, John Ogness wrote:
> On 2022-12-07, Petr Mladek <pmladek@...e.com> wrote:
> >> + /*
> >> + * Append the record text to the dropped message so that it
> >> + * goes out with one write.
> >> + */
> >> + memcpy(ext_text + len, &cbufs->text[0], cmsg->outbuf_len);
> >> +
> >> + /* Update the output buffer descriptor. */
> >> + cmsg->outbuf = ext_text;
> >> + cmsg->outbuf_len += len;
> >
> > I still think that it would be better to rename the buffers in
> > struct console_message and avoid the switches of the purpose
> > of the two buffers.
> >
> > We could print the message about dropped text into a local buffer
> > on stack. IMHO, 64 bytes are acceptable. And we could insert it
> > into the outbuf by shuffling the existing text. Something like:
> >
> > static void msg_print_dropped(struct console_message *cmsg,
> > unsinged long dropped)
> > {
> > char dropped_msg[DROPPED_TEXT_MAX];
> > int dropped_len;
> >
> > if (!con->dropped)
> > return 0;
> >
> > /* Print it into ext_text, which is unused. */
> > dropped_len = snprintf(dropped_msg, sizeof(dropped_msg),
> > "** %lu printk messages dropped **\n", con->dropped);
> >
> > /*
> > * The buffer might already be full only where the message consist
> > * of many very short lines. It is not much realistic.
> > */
> > if (cmsg->outbuf_len + dropped_len + 1 > sizeof(cmsg->outbuf)) {
> > /* Should never happen. */
>
> This certainly can happen. @text is size CONSOLE_LOG_MAX, which is
> LOG_LINE_MAX+PREFIX_MAX. So a totally legal formatted message of length
> LOG_LINE_MAX-1 and a prefix will suddenly become truncated.
>
> > if (WARN_ON_ONCE(dropped_len + 1 > sizeof(cmsg->outbuf)))
> > return;
> >
> > /* Trunk the message like in record_print_text() */
> > cmsg->outbuf_len = sizeof(cmsg->outbuf) - dropped_len;
> > cmsg->outbuf[cmsg->outbuf_len] = '\0';
> > }
> >
> > memmove(cmsg->outbuf + dropped_len, cmsg->outbuf, cmsg->outbuf_len + 1);
> > memcpy(cmsg->outbuf, dropped_msg, dropped_len);
> > }
>
> I do not like the idea of increasing stack usage, possibly cutting off
> messages, and performing extra memory operations all because of some
> variable naming. There is literally a larger unused buffer just sitting
> there.
Sigh. Your approach is copying buffers:
DROPPED_LOG_MAX + CONSOLE_LOG_MAX -> CONSOLE_EXT_LOG_MAX
which means:
64 + 1024 -> 8182
The important thing is that it will shrink the text in
record_print_text() to 1024.
With my approach, it would shrink the text here to 8182 - 64 = 8118.
> I want struct console_buffers to be a black box of working buffers used
> to process the different types of messages. console_get_next_message()
> is the only function that should go inside that black box because it is
> responsible for creating the actual message.
>
> The caller does not care what is in the black box or how those internal
> working buffers are named. The caller just cares about cmsg->outbuf and
> cmsg->outbuf_len, which will point to the data that needs to be written
> out.
>
> For v3 I would like to try my approach one more time. I will give the
> internal buffers new names so as not to mislead their roles. I will
> clearly document the black box nature of struct console_buffers.
This is probably my last mail on this matter[*]. I do not want to get
stuck here. But I really do not see any advantage in your approach:
+ The risk of shrinking the text is bigger.
+ The buffer is accessed via one more dereference that might
eventually point to a wrong buffer if there is a bug.
+ The size of the buffer is not clear via the dereference
and might be wrong if there is a bug.
+ The more layers, the more code complexity, like more names.
The only argument might be the 64B on stack. But it is nothing against
namebuf[KSYM_NAME_LEN] that is used in kallsyms code that might be
called via vsprintf.c. It is 512B on the stack. So I do not take it.
Another argument might be if you wanted to use the buffers yet another
way in the atomic consoles. But I guess (hope) that they will always
work only with the "outbuf".
[*] I think that I'll learn how to live with whatever you use in v3 :-)
Best Regards,
Petr
Powered by blists - more mailing lists