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

Daniil Tatianin
2025-09-15T16:07:57Z

On 9/15/25 6:07 PM, John Ogness wrote:
> 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.

Yeah, I think you're right. I guess the second patch can be dropped now,
which is even better.

>
> 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