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

[Petr Mladek]

On Thu 2022-11-24 22:21:08, John Ogness wrote:
> On 2022-11-24, Petr Mladek <pmladek@xxxxxxxx> 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.

I am not sure if we are talking about the same thing. My idea was to do:

	if (cmsg->is_extmsg) {
		cmsg->outbuf = &cbufs->ext_text[0];
		outbuf_size = sizeof(cbufs->ext_text);
		len = info_print_ext_header(cmsg->outbuf, outbuf_size, r.info);
		len += msg_print_ext_body(cmsg->outbuf + len, outbuf_size - len,
				  &r.text_buf[0], r.info->text_len, &r.info->dev_info);
	} else {
		cmsg->outbuf = &cbufs->text[0];
		/* &r points to &cbufs->text[0], changes are done inline */
		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
	}

> > 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.

Ah, I have overlooked this. It might actually be a motivation to avoid
all these shuffles and really use:

	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.

We handle this this in console_get_next_message() by reading the
messages into the right buffer:

	boot is_extcon = console_srcu_read_flags(con) & CON_EXTENDED;

	/*
	 * Normal consoles might read the message into the outbuf directly.
	 * Console headers are added inplace.
	 */
	if (is_extcon)
		prb_rec_init_rd(&r, &info, &cbufs->readbuf[0], sizeof(cbufs->readbuf));
	else
		prb_rec_init_rd(&r, &info, &cbufs->outbuf[0], sizeof(cbufs->outbuf));

	if (!prb_read_valid(prb, con->seq, &r))
		return false;

	...


	if (is_extcon) {
		len = info_print_ext_header(cbufs->outbuf, sizeof(cbufs->outbuf, r.info);
		len += msg_print_ext_body(cbufs->outbuf + len, sizeof(cbufs->outbuf) - len,
				  &r.text_buf[0], r.info->text_len, &r.info->dev_info);
	} else {
		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
	}



> >      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.

I could imagine adding these metadata into the struct console_buffers.
Or we could call it struct console_messages from the beginning.

We could even completely move con->seq, con->dropped into this new
structure. It would safe even more copies.

IMHO, the less structures and the less copying the better.
Especially when the values have different name in each structure
that makes it even more complicated.

Best Regards,
Petr