Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Nov 2022 10:54:48 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk v2 6/7] printk: Use an output buffer descriptor
 struct for emit
Message-ID: <Y4SFaB5O0JtfQ6MO@alley>
References: <20221123231400.614679-1-john.ogness@linutronix.de>
 <20221123231400.614679-7-john.ogness@linutronix.de>
 <Y3+xK7hHmUIlzq9w@alley>
 <87o7swkqar.fsf@jogness.linutronix.de>
 <Y4CEb7a57PpFpDni@alley>
 <87sfi7s407.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87sfi7s407.fsf@jogness.linutronix.de>
Precedence: bulk

On Fri 2022-11-25 11:55:28, John Ogness wrote:
> On 2022-11-25, Petr Mladek <pmladek@suse.com> wrote:
> >> 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:
> >
> > 	struct console_buffers {
> > 		char	outbuf[CONSOLE_EXT_LOG_MAX];
> > 		char	readbuf[CONSOLE_LOG_MAX];
> > 	};
> >
> > 	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;
> 
> We do not know if there will be dropped messages until _after_ we call
> prb_read_valid().

Yes.

> > 	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);
> > 	}
> 
> And the dropped messages should be inserted now somewhere too.
> 
> The fact is that we need two different sized buffers. Which one is used
> for output is not known in advance. I think it is misleading to name the
> variables based on their purpose because their purpose changes depending
> on the situation.

I still think that changing the purpose of the buffers complicates
the code[*] and is potential source of problems. It might be even
a sign of a bad design. IMHO, it would be a big win if the buffers
have a better defined meaning.

The message about dropped messages is rather short. What about
using a small buffer on stack. And adding it into outbuf
by shuffling the existing message. It is not that complicated.
IMHO, it would be worth it.

> > 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.
> 
> Well, consoles still need to have their own copy of @seq and @dropped
> for proper per-console tracking. But the buffers are globally shared.

Right. I have missed this. OK, what about the following?

struct console_buffers {
	char	outbuf[CONSOLE_EXT_LOG_MAX];
	char	readbuf[CONSOLE_LOG_MAX];
};

struct console_message {
	struct console_buffers *bufs;
	u64 seq;
	unsigned long dropped;
	unsigned int len;  ???
};

struct console {
[...]
	struct console_message *msg;
[...]
};

It will cause that the buffers will be on the 3rd level of
nesting. But I think that we would use the following everywhere
anyway:

void console_func(struct console *con)
{
	char *outbuf = con->bufs->outbuf;

	do_something(outbuf);
}

We could actually even use in console_get_next_message():

	/*
	 * Normal console headers are added inplace. Extended console
	 * headers need to read the messages into a separate buffer.
	 */
	if (is_extcon) {
		readbuf = con->bufs->readbuf;
		redbuf_size = sizeof(con->bufs->readbuf);
	} else {
		readbuf = con->bufs->outbuf;
		redbuf_size = sizeof(con->bufs->outbuf);
	}

IMHO, this should be the only function where this "hack"
would be needed. All others would work just with outbuf.

IMHO, one big advantage is using the same names everywhere.
Another advantage is that it won't be necessary to copy
the values between different structures.

> For this series I will drop @is_extmsg from struct console_message and
> instead make it an argument of console_get_next_message() and
> msg_print_dropped(). That makes more sense at this point. (It needs to
> be a variable because it is not safe to re-read flags multiple times
> when constructing the message.)
> 
> For v3 I will move the two buffers (with whatever name we decide is
> best) into struct console_message and remove the struct console_buffers
> definition. That will also remove the use of @cbufs everywhere.

This looks like a bad idea after all. I forgot that we wanted to share
the buffers between non-atomic consoles. It would be ugly to share
also the metadata, like @seq.

That said, I do not want to get stuck on this. If you still
do not like my proposal feel free to keep the text/text_ext
buffers and struct console_message abstraction. I think that
I could live with it.

Best Regards,
Petr