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:   Thu, 8 Dec 2022 10:29:06 +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
Subject: Re: [PATCH printk v2 7/7] printk: Handle dropped message smarter
Message-ID: <Y5GuYvj6VRnhN0+H@alley>
References: <20221123231400.614679-1-john.ogness@linutronix.de>
 <20221123231400.614679-8-john.ogness@linutronix.de>
 <Y5CMD8iZND3rgugG@alley>
 <87h6y71773.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87h6y71773.fsf@jogness.linutronix.de>
Precedence: bulk

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