[v2] printk_ringbuffer: don't needlessly wrap data blocks around

[PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Daniil Tatianin 5 months, 1 week ago

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.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 kernel/printk/printk_ringbuffer.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index d9fb053cff67..99989a9ce4b4 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -1002,6 +1002,17 @@ static bool desc_reserve(struct printk_ringbuffer *rb, unsigned long *id_out)
 	return true;
 }
 
+static bool is_blk_wrapped(struct prb_data_ring *data_ring,
+			    unsigned long begin_lpos, unsigned long next_lpos)
+{
+	/*
+	 * Subtract one from next_lpos since it's 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)
@@ -1013,7 +1024,7 @@ static unsigned long get_next_lpos(struct prb_data_ring *data_ring,
 	next_lpos = lpos + size;
 
 	/* First check if the data block does not wrap. */
-	if (DATA_WRAPS(data_ring, begin_lpos) == DATA_WRAPS(data_ring, next_lpos))
+	if (!is_blk_wrapped(data_ring, begin_lpos, next_lpos))
 		return next_lpos;
 
 	/* Wrapping data blocks store their data at the beginning. */
@@ -1081,7 +1092,7 @@ static char *data_alloc(struct printk_ringbuffer *rb, unsigned int size,
 	blk = to_block(data_ring, begin_lpos);
 	blk->id = id; /* LMM(data_alloc:B) */
 
-	if (DATA_WRAPS(data_ring, begin_lpos) != DATA_WRAPS(data_ring, next_lpos)) {
+	if (is_blk_wrapped(data_ring, begin_lpos, next_lpos)) {
 		/* Wrapping data blocks store their data at the beginning. */
 		blk = to_block(data_ring, 0);
 
@@ -1125,7 +1136,7 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
 		return NULL;
 
 	/* Keep track if @blk_lpos was a wrapping data block. */
-	wrapped = (DATA_WRAPS(data_ring, blk_lpos->begin) != DATA_WRAPS(data_ring, blk_lpos->next));
+	wrapped = is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next);
 
 	size = to_blk_size(size);
 
@@ -1151,7 +1162,7 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
 
 	blk = to_block(data_ring, blk_lpos->begin);
 
-	if (DATA_WRAPS(data_ring, blk_lpos->begin) != DATA_WRAPS(data_ring, next_lpos)) {
+	if (is_blk_wrapped(data_ring, blk_lpos->begin, next_lpos)) {
 		struct prb_data_block *old_blk = blk;
 
 		/* Wrapping data blocks store their data at the beginning. */
@@ -1187,7 +1198,7 @@ static unsigned int space_used(struct prb_data_ring *data_ring,
 	if (BLK_DATALESS(blk_lpos))
 		return 0;
 
-	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next)) {
+	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next)) {
 		/* Data block does not wrap. */
 		return (DATA_INDEX(data_ring, blk_lpos->next) -
 			DATA_INDEX(data_ring, blk_lpos->begin));
@@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
 	}
 
 	/* Regular data block: @begin less than @next and in same wrap. */
-	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
+	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
 	    blk_lpos->begin < blk_lpos->next) {
 		db = to_block(data_ring, blk_lpos->begin);
 		*data_size = blk_lpos->next - blk_lpos->begin;
 
 	/* Wrapping data block: @begin is one wrap behind @next. */
-	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
-		   DATA_WRAPS(data_ring, blk_lpos->next)) {
+	} else if (!is_blk_wrapped(data_ring,
+		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
 		db = to_block(data_ring, 0);
 		*data_size = DATA_INDEX(data_ring, blk_lpos->next);
 
-- 
2.43.0

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Petr Mladek 3 months, 2 weeks ago

On Fri 2025-09-05 17:41:51, Daniil Tatianin 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.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

JFYI, the patch has been comitted into printk/linux.git,
branch for-6.19.

I have updated the indentation as proposed by John.

Best Regards,
Petr

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Petr Mladek 4 months, 2 weeks ago

On Fri 2025-09-05 17:41:51, Daniil Tatianin 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.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

The patch looks good, especially after understanding the problem
with the maximal record size.

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

I am sorry that I did not wrote this earlier. I am quite confident
with the patch but it is quite tricky. And I do not feel comfortable
with pushing this for 6.18 (the merge window will likely
start in 3 days).

I am going to queue it for 6.19 so that it could get enough
testing in linux-next.

Best Regards,
Petr

PS: There is no need to resend the patch. I could fix the indentation
    when committing it.

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Daniil Tatianin 4 months, 2 weeks ago

On 9/26/25 5:44 PM, Petr Mladek wrote:

> On Fri 2025-09-05 17:41:51, Daniil Tatianin 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.
>>
>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> The patch looks good, especially after understanding the problem
> with the maximal record size.
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Tested-by: Petr Mladek <pmladek@suse.com>
>
> I am sorry that I did not wrote this earlier. I am quite confident
> with the patch but it is quite tricky. And I do not feel comfortable
> with pushing this for 6.18 (the merge window will likely
> start in 3 days).
>
> I am going to queue it for 6.19 so that it could get enough
> testing in linux-next.

Yeah that is fair enough, and no worries.

>
> Best Regards,
> Petr
>
> PS: There is no need to resend the patch. I could fix the indentation
>      when committing it.

Thank you!

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Daniil Tatianin 4 months, 2 weeks ago

Hi all!

Now that the fix for large messages is committed, I think this patch 
should be good to take,
and the second one we can just drop since it's no longer relevant.

Thanks,
Daniil

On 9/5/25 5:41 PM, Daniil Tatianin 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.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
>   kernel/printk/printk_ringbuffer.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index d9fb053cff67..99989a9ce4b4 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1002,6 +1002,17 @@ static bool desc_reserve(struct printk_ringbuffer *rb, unsigned long *id_out)
>   	return true;
>   }
>   
> +static bool is_blk_wrapped(struct prb_data_ring *data_ring,
> +			    unsigned long begin_lpos, unsigned long next_lpos)
> +{
> +	/*
> +	 * Subtract one from next_lpos since it's 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)
> @@ -1013,7 +1024,7 @@ static unsigned long get_next_lpos(struct prb_data_ring *data_ring,
>   	next_lpos = lpos + size;
>   
>   	/* First check if the data block does not wrap. */
> -	if (DATA_WRAPS(data_ring, begin_lpos) == DATA_WRAPS(data_ring, next_lpos))
> +	if (!is_blk_wrapped(data_ring, begin_lpos, next_lpos))
>   		return next_lpos;
>   
>   	/* Wrapping data blocks store their data at the beginning. */
> @@ -1081,7 +1092,7 @@ static char *data_alloc(struct printk_ringbuffer *rb, unsigned int size,
>   	blk = to_block(data_ring, begin_lpos);
>   	blk->id = id; /* LMM(data_alloc:B) */
>   
> -	if (DATA_WRAPS(data_ring, begin_lpos) != DATA_WRAPS(data_ring, next_lpos)) {
> +	if (is_blk_wrapped(data_ring, begin_lpos, next_lpos)) {
>   		/* Wrapping data blocks store their data at the beginning. */
>   		blk = to_block(data_ring, 0);
>   
> @@ -1125,7 +1136,7 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
>   		return NULL;
>   
>   	/* Keep track if @blk_lpos was a wrapping data block. */
> -	wrapped = (DATA_WRAPS(data_ring, blk_lpos->begin) != DATA_WRAPS(data_ring, blk_lpos->next));
> +	wrapped = is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next);
>   
>   	size = to_blk_size(size);
>   
> @@ -1151,7 +1162,7 @@ static char *data_realloc(struct printk_ringbuffer *rb, unsigned int size,
>   
>   	blk = to_block(data_ring, blk_lpos->begin);
>   
> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) != DATA_WRAPS(data_ring, next_lpos)) {
> +	if (is_blk_wrapped(data_ring, blk_lpos->begin, next_lpos)) {
>   		struct prb_data_block *old_blk = blk;
>   
>   		/* Wrapping data blocks store their data at the beginning. */
> @@ -1187,7 +1198,7 @@ static unsigned int space_used(struct prb_data_ring *data_ring,
>   	if (BLK_DATALESS(blk_lpos))
>   		return 0;
>   
> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next)) {
> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next)) {
>   		/* Data block does not wrap. */
>   		return (DATA_INDEX(data_ring, blk_lpos->next) -
>   			DATA_INDEX(data_ring, blk_lpos->begin));
> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>   	}
>   
>   	/* Regular data block: @begin less than @next and in same wrap. */
> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>   	    blk_lpos->begin < blk_lpos->next) {
>   		db = to_block(data_ring, blk_lpos->begin);
>   		*data_size = blk_lpos->next - blk_lpos->begin;
>   
>   	/* Wrapping data block: @begin is one wrap behind @next. */
> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
> +	} else if (!is_blk_wrapped(data_ring,
> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
>   		db = to_block(data_ring, 0);
>   		*data_size = DATA_INDEX(data_ring, blk_lpos->next);
>

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by John Ogness 5 months, 1 week ago

On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index d9fb053cff67..99989a9ce4b4 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>  	}
>  
>  	/* Regular data block: @begin less than @next and in same wrap. */
> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>  	    blk_lpos->begin < blk_lpos->next) {
>  		db = to_block(data_ring, blk_lpos->begin);
>  		*data_size = blk_lpos->next - blk_lpos->begin;
>  
>  	/* Wrapping data block: @begin is one wrap behind @next. */
> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
> +	} else if (!is_blk_wrapped(data_ring,
> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {

It would look nicer if the arguments of the function were indented to
the function parenthesis:

	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
				   DATA_SIZE(data_ring), blk_lpos->next)) {

Otherwise, everything is OK for me.

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

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by John Ogness 5 months, 1 week ago

On 2025-09-05, John Ogness <john.ogness@linutronix.de> wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> index d9fb053cff67..99989a9ce4b4 100644
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>  	}
>>  
>>  	/* Regular data block: @begin less than @next and in same wrap. */
>> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
>> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>>  	    blk_lpos->begin < blk_lpos->next) {
>>  		db = to_block(data_ring, blk_lpos->begin);
>>  		*data_size = blk_lpos->next - blk_lpos->begin;
>>  
>>  	/* Wrapping data block: @begin is one wrap behind @next. */
>> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
>> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
>> +	} else if (!is_blk_wrapped(data_ring,
>> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
>
> It would look nicer if the arguments of the function were indented to
> the function parenthesis:
>
> 	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
> 				   DATA_SIZE(data_ring), blk_lpos->next)) {

Ugh, my mail client messed that up. I meant to write:

	} else if (!is_blk_wrapped(data_ring,
				   blk_lpos->begin + DATA_SIZE(data_ring),
				   blk_lpos->next)) {

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

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Daniil Tatianin 5 months, 1 week ago

On 9/5/25 6:27 PM, John Ogness wrote:
> On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> index d9fb053cff67..99989a9ce4b4 100644
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>   	}
>>   
>>   	/* Regular data block: @begin less than @next and in same wrap. */
>> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
>> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>>   	    blk_lpos->begin < blk_lpos->next) {
>>   		db = to_block(data_ring, blk_lpos->begin);
>>   		*data_size = blk_lpos->next - blk_lpos->begin;
>>   
>>   	/* Wrapping data block: @begin is one wrap behind @next. */
>> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
>> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
>> +	} else if (!is_blk_wrapped(data_ring,
>> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
> It would look nicer if the arguments of the function were indented to
> the function parenthesis:
>
> 	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
> 				   DATA_SIZE(data_ring), blk_lpos->next)) {

Would you like me to resend with this addressed?

Thanks!

> Otherwise, everything is OK for me.
>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> Tested-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by John Ogness 5 months, 1 week ago

On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
> On 9/5/25 6:27 PM, John Ogness wrote:
>> On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>>> index d9fb053cff67..99989a9ce4b4 100644
>>> --- a/kernel/printk/printk_ringbuffer.c
>>> +++ b/kernel/printk/printk_ringbuffer.c
>>> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>>   	}
>>>   
>>>   	/* Regular data block: @begin less than @next and in same wrap. */
>>> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
>>> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>>>   	    blk_lpos->begin < blk_lpos->next) {
>>>   		db = to_block(data_ring, blk_lpos->begin);
>>>   		*data_size = blk_lpos->next - blk_lpos->begin;
>>>   
>>>   	/* Wrapping data block: @begin is one wrap behind @next. */
>>> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
>>> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
>>> +	} else if (!is_blk_wrapped(data_ring,
>>> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
>> It would look nicer if the arguments of the function were indented to
>> the function parenthesis:
>>
>> 	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
>> 				   DATA_SIZE(data_ring), blk_lpos->next)) {
>
> Would you like me to resend with this addressed?

Knowing Petr, I would say "yes". :-)

But wait for Petr's response before sending anything.

John

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Daniil Tatianin 5 months ago

On 9/5/25 7:10 PM, John Ogness wrote:
> On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>> On 9/5/25 6:27 PM, John Ogness wrote:
>>> On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>>>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>>>> index d9fb053cff67..99989a9ce4b4 100644
>>>> --- a/kernel/printk/printk_ringbuffer.c
>>>> +++ b/kernel/printk/printk_ringbuffer.c
>>>> @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>>>    	}
>>>>    
>>>>    	/* Regular data block: @begin less than @next and in same wrap. */
>>>> -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
>>>> +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
>>>>    	    blk_lpos->begin < blk_lpos->next) {
>>>>    		db = to_block(data_ring, blk_lpos->begin);
>>>>    		*data_size = blk_lpos->next - blk_lpos->begin;
>>>>    
>>>>    	/* Wrapping data block: @begin is one wrap behind @next. */
>>>> -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
>>>> -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
>>>> +	} else if (!is_blk_wrapped(data_ring,
>>>> +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
>>> It would look nicer if the arguments of the function were indented to
>>> the function parenthesis:
>>>
>>> 	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
>>> 				   DATA_SIZE(data_ring), blk_lpos->next)) {
>> Would you like me to resend with this addressed?
> Knowing Petr, I would say "yes". :-)
>
> But wait for Petr's response before sending anything.

Friendly ping for Petr :)

>
> John

Re: [PATCH v2 1/2] printk_ringbuffer: don't needlessly wrap data blocks around

Posted by Petr Mladek 5 months ago

On Thu 2025-09-11 11:34:54, Daniil Tatianin wrote:
> 
> On 9/5/25 7:10 PM, John Ogness wrote:
> > On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
> > > On 9/5/25 6:27 PM, John Ogness wrote:
> > > > On 2025-09-05, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
> > > > > diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> > > > > index d9fb053cff67..99989a9ce4b4 100644
> > > > > --- a/kernel/printk/printk_ringbuffer.c
> > > > > +++ b/kernel/printk/printk_ringbuffer.c
> > > > > @@ -1234,14 +1245,14 @@ static const char *get_data(struct prb_data_ring *data_ring,
> > > > >    	}
> > > > >    	/* Regular data block: @begin less than @next and in same wrap. */
> > > > > -	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next) &&
> > > > > +	if (!is_blk_wrapped(data_ring, blk_lpos->begin, blk_lpos->next) &&
> > > > >    	    blk_lpos->begin < blk_lpos->next) {
> > > > >    		db = to_block(data_ring, blk_lpos->begin);
> > > > >    		*data_size = blk_lpos->next - blk_lpos->begin;
> > > > >    	/* Wrapping data block: @begin is one wrap behind @next. */
> > > > > -	} else if (DATA_WRAPS(data_ring, blk_lpos->begin + DATA_SIZE(data_ring)) ==
> > > > > -		   DATA_WRAPS(data_ring, blk_lpos->next)) {
> > > > > +	} else if (!is_blk_wrapped(data_ring,
> > > > > +		   blk_lpos->begin + DATA_SIZE(data_ring), blk_lpos->next)) {
> > > > It would look nicer if the arguments of the function were indented to
> > > > the function parenthesis:
> > > > 
> > > > 	} else if (!is_blk_wrapped(data_ring, blk_lpos->begin +
> > > > 				   DATA_SIZE(data_ring), blk_lpos->next)) {
> > > Would you like me to resend with this addressed?
> > Knowing Petr, I would say "yes". :-)
> > 
> > But wait for Petr's response before sending anything.
> 
> Friendly ping for Petr :)

I am sorry for the delay. I actually was going to look at it today.
But to make expectations. I am a bit overloaded by other tasks.
And frankly, this patchset is not a big priority. It allows
to use 4 unused bytes which is a negligible win. And it stretches
the limits which might open a hole which was closed by chance before.

Best Regards,
Petr