Re: [PATCH v4 01/13] ring-buffer: Allow mapped field to be set without mapping

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 11 Jun 2024 16:53:43 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> >>> @@ -6403,7 +6407,8 @@ int ring_buffer_unmap(struct trace_buffer *buffer, int cpu)
> >>>    	mutex_lock(&buffer->mutex);
> >>>    	raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> >>>    
> >>> -	cpu_buffer->mapped = 0;
> >>> +	WARN_ON_ONCE(!cpu_buffer->mapped);
> >>> +	cpu_buffer->mapped--;  
> >>
> >> This will wrap to UINT_MAX if it was 0. Is that intentional ?  
> > 
> > If mapped is non zero, it limits what it can do. If it enters here as zero,
> > we are really in a unknown state, so yeah, wrapping will just keep it
> > limited. Which is a good thing.
> > 
> > Do you want me to add a comment there?
> >   
> 
> Maybe. I just wondered if something like
> 	if (!WARN_ON_ONCE(!cpu_buffer->mapped))
> 		cpu_buffer->mapped--;
> 
> would be better than wrapping because 'mapped' is used as flag elsewhere,
> but then I can see that it is also manipulated in __rb_inc_dec_mapped(),
> and that it is checked against UINT_MAX there (and not decremented if it is 0).

Yeah, the __rb_inc_dec_mapped() is used as it is called when external
sources map the ring buffer. 

This is incremented and decremented internally. That is, we increment
it the first time the ring buffer is mapped, and decrement it again the
last time it is mapped.

I could add the above logic as well. I hit a bug in my more vigorous
testing so I need to make another revision anyway.

> 
> Maybe explain why sometimes __rb_inc_dec_mapped() is called to
> increment or decrement ->mapped, and sometimes it id done directly ?
> I can see that the function also acquires the buffer mutex, which
> isn't needed at the places where mapped is incremented/decremented
> directly, but common code would still be nice, and it is odd to see
> over/underflows handled sometimes but not always.

Sure. I'll add comments explaining more.

Thanks,

-- Steve