Received-SPF: pass (google.com: domain of linux-kernel+bounces-191515-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 27 May 2024 19:43:56 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Petr Pavlu <petr.pavlu@suse.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@efficios.com>, linux-trace-kernel@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-rt-users
 <linux-rt-users@vger.kernel.org>
Subject: Re: [PATCH 2/2] ring-buffer: Fix a race between readers and resize
 checks
Message-ID: <20240527194356.5078b56f@rorschach.local.home>
In-Reply-To: <2b920bab-23a2-4a8d-95c2-b69472d38373@suse.com>
References: <20240517134008.24529-1-petr.pavlu@suse.com>
	<20240517134008.24529-3-petr.pavlu@suse.com>
	<20240520095037.33a7fde6@gandalf.local.home>
	<2b920bab-23a2-4a8d-95c2-b69472d38373@suse.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 27 May 2024 11:36:55 +0200
Petr Pavlu <petr.pavlu@suse.com> wrote:

> >>  static void rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer)
> >>  {
> >> @@ -2200,8 +2205,13 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
> >>  		 */
> >>  		synchronize_rcu();
> >>  		for_each_buffer_cpu(buffer, cpu) {
> >> +			unsigned long flags;
> >> +
> >>  			cpu_buffer = buffer->buffers[cpu];
> >> +			raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> >>  			rb_check_pages(cpu_buffer);
> >> +			raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock,
> >> +						   flags);  
> > 
> > Putting my RT hat on, I really don't like the above fix. The
> > rb_check_pages() iterates all subbuffers which makes the time interrupts
> > are disabled non-deterministic.  
> 
> I see, this applies also to the same rb_check_pages() validation invoked
> from ring_buffer_read_finish().
> 
> > 
> > Instead, I would rather have something where we disable readers while we do
> > the check, and re-enable them.
> > 
> > 			raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> > 			cpu_buffer->read_disabled++;
> > 			raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> > 
> > // Also, don't put flags on a new line. We are allow to go 100 characters now.  
> 
> Noted.
> 
> > 
> > 
> >   			rb_check_pages(cpu_buffer);
> > 			raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
> > 			cpu_buffer->read_disabled--;
> > 			raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> > 
> > Or something like that. Yes, that also requires creating a new
> > "read_disabled" field in the ring_buffer_per_cpu code.  
> 
> I think this would work but I'm personally not immediately sold on this
> approach. If I understand the idea correctly, readers should then check
> whether cpu_buffer->read_disabled is set and bail out with some error if
> that is the case. The rb_check_pages() function is only a self-check
> code and as such, I feel it doesn't justify disrupting readers with
> a new error condition and adding more complex locking.

Honestly, this code was never made for more than one reader per
cpu_buffer. I'm perfectly fine if all check_pages() causes other
readers to the same per_cpu buffer to get -EBUSY.

Do you really see this being a problem? What use case is there for
hitting the check_pages() and reading the same cpu buffer at the same
time?

-- Steve