Re: [PATCH printk v2 6/7] printk: Use an output buffer descriptor struct for emit

John Ogness
2022-11-24T21:15:23Z

On 2022-11-24, Petr Mladek <pmladek@suse.com> wrote:
> I wish, this change was done in two patches. 1st introducing and
> using struct console_message. 2nd moving the code into separate
> console_get_next_message().

OK.

>> + if (cmsg->is_extmsg) {
>> + write_text = &cbufs->ext_text[0];
>> + write_text_size = sizeof(cbufs->ext_text);
>> + len = info_print_ext_header(write_text, write_text_size, r.info);
>> + len += msg_print_ext_body(write_text + len, write_text_size - len,
>> + &r.text_buf[0], r.info->text_len, &r.info->dev_info);
>> + } else {
>> + write_text = &cbufs->text[0];
>> + len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
>> + }
>> +
>> + cmsg->outbuf = write_text;
>> + cmsg->outbuf_len = len;
>
> Please, remove "write_text" variable and use cmsg->outbuf directly.
> It would safe one mental transition of buffer names:
>
> cbufs->text -> write_text -> cmsg->outbuf
>
> vs.
>
> cbufs->text -> cmsg->outbuf

I originally had the non-extended case without @write_text. I felt like
it was harder to follow what actually got set. Really the main objective
of the function is to set @outbuf and @outbuf_len. I felt like moving
that outside of the if/else block made it clearer what is going on. But
I can go back to having each if/else branch set those fields in their
own way.

> PS: Please, wait a bit with updating the patches. I have got yet
> another idea when seeing the code around dropped messages.
> But I have to sleep over it.

Don't worry. I always wait until you finish the full review before
touching anything. ;-)

> My concern is that the message about dropped messages need not
> fit into the smaller "cbufs->text" buffer. It might be better
> to put it into the bigger one.

This series _does_ put the dropped messages in the bigger one.

> We might actually always use the bigger buffer as the output
> buffer. The smaller buffer might be only temporary when formatting
> the extended messages.
>
> We could replace
>
> struct console_buffers {
> char ext_text[CONSOLE_EXT_LOG_MAX];
> char text[CONSOLE_LOG_MAX];
> };
>
> with
>
> struct console_buffers {
> char outbuf[CONSOLE_EXT_LOG_MAX];
> char readbuf[CONSOLE_LOG_MAX];
> };
>
> Normal consoles would use only @outbuf. Only the extended console
> would need the @readbuf to read the messages before they are
> formatted.

The "problem" with this idea is that record_print_text() creates the
normal output in-place within the readbuf. So for normal messages with
no dropped messages, we still end up writing out the readbuf.

> I guess that struct console_message won't be needed then at all.

Since we sometimes output the in-place readbuf and sometimes a newly
written buffer, it is nice that console_message can abstract that out.

Also, right now @is_extmsg is the only input variable. For thread/atomic
consoles, the input variables @seq and @dropped will be added.
console_message will then have its own copy of all the information
needed to let itself get filled and console_get_next_message() will no
longer require the console as an argument.

This is important for the thread/atomic consoles because it removes all
locking constraints from console_get_next_message(). For _this_ series,
console_get_next_message() still requires holding the console_lock
because it is accessing con->seq and con->dropped.

I could have added @seq and @dropped to console_message for this series,
but for the legacy consoles it looks like a lot of unnecessary
copying. Only with the thread/atomic consoles does the benefit become
obvious.

> It might help to remove several twists in the code.

A lot of this is preparation for the thread/atomic consoles. It is a
little bit twisty because it is primarily designed for the new
consoles. The trick is to get us from old to new without things getting
crazy in between.

I appreciate your comments. You see things from the perspective of the
"legacy consoles", which is helpful for us to keep things clean and
maintainable during the transition.

John