Messages in this thread | | | Date | Thu, 8 Dec 2022 10:29:06 +0100 | From | Petr Mladek <> | 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@suse.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
| |