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, 10 Oct 2022 18:07:18 +0200
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 18/18] printk: Handle dropped message smarter
Message-ID: <Y0RDNjyYh0BpDiIQ@alley>
References: <20220924000454.3319186-1-john.ogness@linutronix.de>
 <20220924000454.3319186-19-john.ogness@linutronix.de>
 <YzEoYPSC5Qf2aL92@google.com>
 <87leq6d0zn.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87leq6d0zn.fsf@jogness.linutronix.de>
Precedence: bulk

On Mon 2022-09-26 10:00:36, John Ogness wrote:
> On 2022-09-26, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> > On (22/09/24 02:10), John Ogness wrote:
> >> +/**
> >> + * cons_print_dropped - Print 'dropped' message if required
> >> + * @desc:	Pointer to the output descriptor
> >> + *
> >> + * Prints the 'dropped' message info the output buffer if @desc->dropped is
> >> + * not 0 and the regular format is requested. Extended format does not
> >> + * need this message because it prints the sequence numbers.
> >> + *
> >> + * In regular format the extended message buffer is not in use.
> >> + * So print into it at the beginning and move the resulting string
> >> + * just in front of the regular text buffer so that the message can
> >> + * be printed in one go.
> >> + *
> >> + * In case of a message this returns with @desc->outbuf and @desc->len
> >> + * updated. If no message is required then @desc is not modified.
> >> + */
> >> +static void cons_print_dropped(struct cons_outbuf_desc *desc)
> >> +{
> >> +	struct cons_text_buf *txtbuf = desc->txtbuf;
> >> +	size_t len;
> >> +
> >> +	if (!desc->dropped || desc->extmsg)
> >> +		return;
> >> +
> >> +	if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
> >> +		return;
> >> +
> >> +	/* Print it into ext_text which is unused */
> >> +	len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
> >> +		       "** %lu printk messages dropped **\n", desc->dropped);
> >> +	desc->dropped = 0;
> >> +
> >> +	/* Copy it just below text so it goes out with one write */
> >> +	memcpy(txtbuf->text - len, txtbuf->ext_text, len);
> >> +
> >> +	/* Update the descriptor */
> >> +	desc->len += len;
> >> +	desc->outbuf -= len;
> >
> > Oh, hmm. This does not look to me as a simplification. Quite
> > the opposite, moving cons_text_buf::text pointer to point to
> > cons_text_buf::text - strlen("... dropped messages...") looks
> > somewhat fragile.
> 
> It relies on @ext_text and @text being packed together, which yes, may
> be fragile.

Yes, it is a nasty hack ;-)

I suggest to increase CONSOLE_LOG_MAX to 2048,
define LOG_LINE_MAX as 1024, and use the buffer for both
dropped message and normal message.

It would simplify the code. Also it would make enough
space for more potential line headers needed by more
lines in one record.

It would require moving the normal message to make a space for
the dropped messages. But the dropping should be rare. And we
do a lot of moving in record_print_text() anyway.

I think that I was against increasing the buffer size some time ago.
I was worried about small devices. But I think that the patch
just increased the buffer size without any bug report so that
the justification was weak.

But simplifying the code looks like a good justification to me.
And I really like the removal of the extra buffer.

Best Regards,
Petr