Re: [PATCH 2/2] printk_ringbuffer: Create a helper function to decide whether a more space is needed

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

Hi Petr,

Nit: For the patch subject, remove the word "a":

"Create a helper function to decide whether more space is needed"

More below...

On 2025-11-07, Petr Mladek <pmladek@suse.com> wrote:
> The decision whether some more space is needed is tricky in the printk
> ring buffer code:
>
>   1. The given lpos values might overflow. A subtraction must be used
>      instead of a simple "lower than" check.
>
>   2. Another CPU might reuse the space in the mean time. It can be
>      detected when the subtraction is bigger than DATA_SIZE(data_ring).
>
>   3. There is exactly enough space when the result of the subtraction
>      is zero. But more space is needed when the result is exactly
>      DATA_SIZE(data_ring).
>
> Add a helper function to make sure that the check is done correctly
> in all situations. Also it helps to make the code consistent and
> better documented.
>
> Suggested-by: John Ogness <john.ogness@linutronix.de>
> Link: https://lore.kernel.org/r/87tsz7iea2.fsf@jogness.linutronix.de
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/printk/printk_ringbuffer.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 3e6fd8d6fa9f..ede3039dd041 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -411,6 +411,23 @@ static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
>  	return to_blk_size(size) <= DATA_SIZE(data_ring) / 2;
>  }
>  
> +/*
> + * Compare the current and requested logical position and decide
> + * whether more space needed.
> + *
> + * Return false when @lpos_current is already at or beyond @lpos_target.
> + *
> + * Also return false when the difference between the positions is bigger
> + * than the size of the data buffer. It might happen only when the caller
> + * raced with another CPU(s) which already made and used the space.
> + */
> +static bool need_more_space(struct prb_data_ring *data_ring,
> +			    unsigned long lpos_current,
> +			    unsigned long lpos_target)
> +{
> +	return lpos_target - lpos_current - 1 < DATA_SIZE(data_ring);
> +}
> +
>  /* Query the state of a descriptor. */
>  static enum desc_state get_desc_state(unsigned long id,
>  				      unsigned long state_val)
> @@ -577,7 +594,7 @@ static bool data_make_reusable(struct printk_ringbuffer *rb,
>  	unsigned long id;
>  
>  	/* Loop until @lpos_begin has advanced to or beyond @lpos_end. */
> -	while ((lpos_end - lpos_begin) - 1 < DATA_SIZE(data_ring)) {
> +	while (need_more_space(data_ring, lpos_begin, lpos_end)) {
>  		blk = to_block(data_ring, lpos_begin);
>  
>  		/*
> @@ -668,7 +685,7 @@ static bool data_push_tail(struct printk_ringbuffer *rb, unsigned long lpos)
>  	 * sees the new tail lpos, any descriptor states that transitioned to
>  	 * the reusable state must already be visible.
>  	 */
> -	while ((lpos - tail_lpos) - 1 < DATA_SIZE(data_ring)) {
> +	while (need_more_space(data_ring, tail_lpos, lpos)) {
>  		/*
>  		 * Make all descriptors reusable that are associated with
>  		 * data blocks before @lpos.
> @@ -1148,8 +1165,14 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
>  
>  	next_lpos = get_next_lpos(data_ring, blk_lpos->begin, size);
>  
> -	/* If the data block does not increase, there is nothing to do. */
> -	if (head_lpos - next_lpos < DATA_SIZE(data_ring)) {
> +	/*
> +	 * Use the current data block when the size does not increase.

I would like to expand the above sentence so that it is a bit clearer
how it relates to the new check. Perhaps:

	 * Use the current data block when the size does not increase, i.e.
	 * when @head_lpos is already able to accommodate the new @next_lpos.

> +	 *
> +	 * Note that need_more_space() could never return false here because
> +	 * the difference between the positions was bigger than the data
> +	 * buffer size. The data block is reopened and can't get reused.
> +	 */
> +	if (!need_more_space(data_ring, head_lpos, next_lpos)) {
>  		if (wrapped)
>  			blk = to_block(data_ring, 0);
>  		else
> -- 
> 2.51.1

Otherwise, LGTM. Thanks for choosing a name that presents contextual
purpose rather than simply function.

Reviewed-by: John Ogness <john.ogness@linutronix.de>