public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: John Ogness <john.ogness@linutronix.de>
To: Daniil Tatianin <d-tatianin@yandex-team.ru>,
	Petr Mladek <pmladek@suse.com>
Cc: linux-kernel@vger.kernel.org,
	Steven Rostedt <rostedt@goodmis.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around
Date: Mon, 15 Sep 2025 17:13:03 +0206	[thread overview]
Message-ID: <84wm5z7o14.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <fece29ff-070e-4074-85be-4093a3000e5d@yandex-team.ru>

On 2025-09-14, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>> After applying your patch, can you provide an example where a maximum
>> size of exactly half causes the tail to be pushed beyond the head? Keep
>> in mind that data_check_size() accounts for the meta-data. It only
>> doesn't account for the extra ID on wrapping data blocks.
>
> Sorry, I think exactly half is fine, basically we can keep it half, but 
> only remove the tailing id check with my patch.

I have been investigating this further. Even _without_ your patches, I
cannot find (either by using my brain or through testing) a problem with
limiting it to exactly half:

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index bc811de18316b..9d47c1b94b71f 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -398,8 +398,6 @@ static unsigned int to_blk_size(unsigned int size)
  */
 static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 {
-	struct prb_data_block *db = NULL;
-
 	if (size == 0)
 		return true;
 
@@ -408,11 +406,7 @@ static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 	 * array. The largest possible data block must still leave room for
 	 * at least the ID of the next block.
 	 */
-	size = to_blk_size(size);
-	if (size > DATA_SIZE(data_ring) - sizeof(db->id))
-		return false;
-
-	return true;
+	return (to_blk_size(size) <= (DATA_SIZE(data_ring) / 2));
 }
 
 /* Query the state of a descriptor. */

When invalidating a data block (pushing the tail) it only must be
certain that the newly created space is large enough to fit the new data
block.

With a maximum of half, a new non-wrapping data block will always
fit. If it is a wrapping data block the worst case is if it is maximally
sized and ends exactly at the end of the array. In the case, it is
placed at index 0. But there it will only free up until the head
value. (If the head value was less, the data block would not have
wrapped.)

Your series handles the "ends exactly at the end of the array" case by
avoiding the need to wrap and thus invalidate up to half the
ringbuffer. But your series does not affect the maximum record size.

I will submit an official patch that also improves the comments to
clarify exactly why the limit exists.

@Petr: I am fine with you keeping our 1/4 limit in printk.c. But I would
like the ringbuffer code to be exactly proper here.

John

  reply	other threads:[~2025-09-15 15:07 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-05 14:41 [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around Daniil Tatianin
2025-09-05 14:41 ` [PATCH v2 1/2] " Daniil Tatianin
2025-09-05 15:27   ` John Ogness
2025-09-05 15:29     ` Daniil Tatianin
2025-09-05 16:10       ` John Ogness
2025-09-11  8:34         ` Daniil Tatianin
2025-09-11 15:33           ` Petr Mladek
2025-09-05 15:30     ` John Ogness
2025-09-26 14:35   ` Daniil Tatianin
2025-09-26 14:44   ` Petr Mladek
2025-09-26 14:53     ` Daniil Tatianin
2025-10-22 12:46   ` Petr Mladek
2025-09-05 14:41 ` [PATCH v2 2/2] printk_ringbuffer: allow one data block to occupy the entire data ring Daniil Tatianin
2025-09-05 15:27   ` John Ogness
2025-09-11 15:30 ` [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around Petr Mladek
2025-09-11 15:55   ` Petr Mladek
2025-09-11 16:11     ` Petr Mladek
2025-09-11 15:58   ` Petr Mladek
2025-09-11 16:12     ` John Ogness
2025-09-12  9:25       ` Petr Mladek
2025-09-12  9:54         ` Petr Mladek
2025-09-12 14:49           ` Petr Mladek
2025-09-12 15:15             ` Petr Mladek
2025-09-12 18:43             ` John Ogness
2025-09-13 17:38               ` Daniil Tatianin
2025-09-14  9:23                 ` John Ogness
2025-09-14  9:56                   ` Daniil Tatianin
2025-09-15 15:07                     ` John Ogness [this message]
2025-09-15 16:00                       ` Petr Mladek
2025-09-15 16:07                       ` Daniil Tatianin
2025-09-15 15:08               ` Petr Mladek
2025-09-15 15:25                 ` John Ogness

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=84wm5z7o14.fsf@jogness.linutronix.de \
    --to=john.ogness@linutronix.de \
    --cc=d-tatianin@yandex-team.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox