Re: [PATCH v1] printk_ringbuffer: don't needlessly wrap data blocks around

[John Ogness <john.ogness@linutronix.de>]

On 2025-09-03, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
> Previously, data blocks that perfectly fit the data ring buffer would
> get wrapped around to the beginning for no reason since the calculated
> offset of the next data block would belong to the next wrap. Since this
> offset is not actually part of the data block, but rather the offset of
> where the next data block is going to start, there is no reason to
> include it when deciding whether the current block fits the buffer.

This is a nice catch!

Although note that this patch avoids wasting a maximum of 8 bytes of
ringbuffer space. If you are interested in tackling the wasted-space
issue of the printk ringbuffer there are much larger [0] fish to catch.

[0] https://lore.kernel.org/lkml/84y10vz7ty.fsf@jogness.linutronix.de

My comments below...

> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index d9fb053cff67..f885ba8be5e6 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1002,6 +1002,18 @@ static bool desc_reserve(struct printk_ringbuffer *rb, unsigned long *id_out)
>  	return true;
>  }
>  
> +static bool same_lpos_wraps(struct prb_data_ring *data_ring,
> +			     unsigned long begin_lpos, unsigned long next_lpos)

We need a better name here since it is not actually using the value of
@next_lpos to check the wrap count. Perhaps inverting the return value
and naming it blk_lpos_wraps(). So it would be identifying if the given
blk_lpos values lead to a wrapping data block. Some like this:

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index d9fb053cff67d..cf0fcd9b106ae 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -1002,6 +995,17 @@ static bool desc_reserve(struct printk_ringbuffer *rb, unsigned long *id_out)
 	return true;
 }
 
+/* Identify if given blk_lpos values represent a wrapping data block. */
+static bool blk_lpos_wraps(struct prb_data_ring *data_ring,
+			   unsigned long begin_lpos, unsigned long next_lpos)
+{
+	/*
+	 * Subtract one from @next_lpos since it is not actually part of this
+	 * data block. This allows perfectly fitting records to not wrap.
+	 */
+	return (DATA_WRAPS(data_ring, begin_lpos) != DATA_WRAPS(data_ring, next_lpos - 1));
+}
+
 /* Determine the end of a data block. */
 static unsigned long get_next_lpos(struct prb_data_ring *data_ring,
 				   unsigned long lpos, unsigned int size)

> +{
> +	/*
> +	 * Subtract one from next_lpos since it's not actually part of this data
> +	 * block. We do this to prevent perfectly fitting records from wrapping
> +	 * around for no reason.
> +	 */
> +	return DATA_WRAPS(data_ring, begin_lpos) ==
> +	       DATA_WRAPS(data_ring, next_lpos - 1);
> +}
> +
>  /* Determine the end of a data block. */
>  static unsigned long get_next_lpos(struct prb_data_ring *data_ring,
>  				   unsigned long lpos, unsigned int size)

The rest looked fine to me and also passed various private
tests. However, we should also update data_check_size(), since now data
blocks are allowed to occupy the entire data ring. Something like this:

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index d9fb053cff67d..e6bdfb8237a3d 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -397,21 +397,14 @@ 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;
 
 	/*
 	 * Ensure the alignment padded size could possibly fit in the data
-	 * array. The largest possible data block must still leave room for
-	 * at least the ID of the next block.
+	 * array.
 	 */
-	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));
 }
 
 /* Query the state of a descriptor. */

Petr may have other ideas/opinions about this, so you should wait for a
response from him before submitting a new version.

John