Re: [PATCH 1/6] ring-buffer: Fix subbuf resize concurrency

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 10 Aug 2026 13:56:28 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:



> +static __always_inline unsigned int rb_subbuf_size(struct trace_buffer *buffer)
> +{
> +	return PAGE_SIZE << buffer->subbuf_order;
> +}



> @@ -3513,7 +3524,7 @@ rb_event_index(struct ring_buffer_per_cpu *cpu_buffer, struct ring_buffer_event
>  {
>  	unsigned long addr = (unsigned long)event;
>  
> -	addr &= (PAGE_SIZE << cpu_buffer->buffer->subbuf_order) - 1;
> +	addr &= (unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1;

This one is fine because it already sits in a helper function.

>  
>  	return addr - BUF_PAGE_HDR_SIZE;
>  }

> @@ -4102,7 +4113,7 @@ rb_try_to_discard(struct ring_buffer_per_cpu *cpu_buffer,
>  	new_index = rb_event_index(cpu_buffer, event);
>  	old_index = new_index + rb_event_ts_length(event);
>  	addr = (unsigned long)event;
> -	addr &= ~((PAGE_SIZE << cpu_buffer->buffer->subbuf_order) - 1);
> +	addr &= ~((unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1);
>  
>  	bpage = READ_ONCE(cpu_buffer->tail_page);
>  

> @@ -5012,7 +5023,7 @@ rb_decrement_entry(struct ring_buffer_per_cpu *cpu_buffer,
>  	struct buffer_page *bpage = cpu_buffer->commit_page;
>  	struct buffer_page *start;
>  
> -	addr &= ~((PAGE_SIZE << cpu_buffer->buffer->subbuf_order) - 1);
> +	addr &= ~((unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1);
>  
>  	/* Do the likely case first */
>  	if (likely(bpage->page == (void *)addr)) {


I really hate the above open coded typecasting to get the address correct.
Seems very fragile to me. As it is getting the address of the sub buffer,
let's add another helper function:

/**
 * rb_subbuf_addr - Return the address of the start of a subbuffer
 * @cpu_buffer: The cpu buffer that @addr is on
 * @addr: An address of an event on a subbuffer
 *
 * Returns: The start of the subbuffer for where @addr sits
 */
static __always_inline unsigned long rb_subbuf_addr(struct ring_buffer_per_cpu *cpu_buffer,
						    unsigned long addr)
{
	return addr & ~((unsigned long)(rb_subbuf_size(cpu_buffer->buffer) - 1));
}

And use that for these locatons.

-- Steve