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

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Aug 10, 2026 at 09:26:06AM -0400, Steven Rostedt wrote:
> On Mon, 10 Aug 2026 08:54:22 +0100
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > This is probably the issue.
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index c2cbdb3ef727..a64cdc925078 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -3534,7 +3534,7 @@ rb_event_index(struct ring_buffer_per_cpu *cpu_buffer, struct ring_buffer_event
> >  {
> >         unsigned long addr = (unsigned long)event;
> >  
> > -       addr &= rb_subbuf_size(cpu_buffer->buffer) - 1;
> > +       addr &= (unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1;
> >  
> >         return addr - BUF_PAGE_HDR_SIZE;
> >  }
> > @@ -4123,7 +4123,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 &= ~(rb_subbuf_size(cpu_buffer->buffer) - 1);
> > +       addr &= ~((unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1);
> >  
> >         bpage = READ_ONCE(cpu_buffer->tail_page);
> >  
> > @@ -5033,7 +5033,7 @@ rb_decrement_entry(struct ring_buffer_per_cpu *cpu_buffer,
> >         struct buffer_page *bpage = cpu_buffer->commit_page;
> >         struct buffer_page *start;
> >  
> > -       addr &= ~(rb_subbuf_size(cpu_buffer->buffer) - 1);
> > +       addr &= ~((unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1);
> 
> What about just making those functions return unsigned long instead of
> having to add typecasts all over. It shouldn't hurt the places it's made an
> int? Although I'm not sure if bots will complain. :-/

I thought about that, but it is used to assign to 32-bits variables, such as
ring_buffer_cpu_meta::subbuf_size or in ring_buffer_subbuf_size_get().

so it felt the (unsigned long) cast wasn't that bad in the end.

Alternatively, I could make all the "size" (_size, _capacity _max_data_size)
related accessors "unsigned long" and do the same for the buffer_data_page and
buffer_page accessors?

And just WARN() if we set one of the meta data fields with more than U32_MAX?

> 
> >  
> >         /* Do the likely case first */
> >         if (likely(bpage->page == (void *)addr)) {
> > 
> > 
> > I will resend a V3 dropping the 3 first patches you have applied already.
> 
> Yeah, they are already in Linus's tree.
> 
> -- Steve

I have sent the v3:

https://lore.kernel.org/all/20260810125633.3344684-1-vdonnefort@google.com/

-- 
Vincent