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;
From:   John Ogness <john.ogness@linutronix.de>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Petr Mladek <pmladek@suse.com>,
        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
In-Reply-To: <YzEoYPSC5Qf2aL92@google.com>
References: <20220924000454.3319186-1-john.ogness@linutronix.de>
 <20220924000454.3319186-19-john.ogness@linutronix.de>
 <YzEoYPSC5Qf2aL92@google.com>
Date:   Mon, 26 Sep 2022 10:00:36 +0206
Message-ID: <87leq6d0zn.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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. As an alternative we could memcpy the message text (@text)
to the end of the dropped message text. There would be enough room.

Generally speaking, the dropped text will be less text to copy. But
since dropped messages are rare anyway, it might be worth copying more
data so that the code is not fragile. It would also allow us to remove
the __no_randomize_layout in "struct cons_text_buf".

If the end of cons_print_dropped was changed to:

        memcpy(txtbuf->ext_text + len, txtbuf->text, desc->len);
        desc->len += len;
        desc->outbuf = txtbuf->ext_text;

Would that be OK for you?

> Is printing 'dropped' and outbuf messages in one go such an
> important feature?

I think it is a nice simplification. With the cons_text_buf, it makes it
quite easy to implement.

John