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

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

On 2025-09-04, Petr Mladek <pmladek@suse.com> wrote:
> On Thu 2025-09-04 16:04:30, John Ogness wrote:
>> > +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.
>
> Or a combination of my and this proposal: is_blk_wrapped().

Sounds good to me.

>> 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));
>>  }
>
> I hope that we would never reach this limit. A buffer for one
> message does not look practical. I originally suggested to avoid
> messages bigger than 1/4 of the buffer size ;-)
>
> That said, strictly speaking, the above change looks correct.
> I would just do it in a separate patch. The use of the full
> buffer and the limit of the maximal message are related
> but they are not the same things. Also separate patch might
> help with bisection in case of problems.

FWIW, Aside from inspecting all the related code carefully, I also
performed various size and boundary tests using small buffers (like 1KB
and 2KB). I am confortable with these changes.

John