Received-SPF: pass (google.com: domain of linux-kernel+bounces-88807-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: Fri, 1 Mar 2024 12:11:21 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: linke <lilinke99@qq.com>, paulmck <paulmck@kernel.org>,
 linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
 mhiramat@kernel.org, Peter Zijlstra <peterz@infradead.org>, Josh Triplett
 <josh@joshtriplett.org>, Linus Torvalds <torvalds@linux-foundation.org>,
 Andrew Morton <akpm@linux-foundation.org>, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] ring-buffer: use READ_ONCE() to read
 cpu_buffer->commit_page in concurrent environment
Message-ID: <20240301121121.47b4fbdd@gandalf.local.home>
In-Reply-To: <fde2e34a-2f5f-4bee-8f6f-60acf2e0d4a3@efficios.com>
References: <tencent_3F31ADAB6142D8948E5BD02B854ED2A77106@qq.com>
	<20240301104945.43119349@gandalf.local.home>
	<fde2e34a-2f5f-4bee-8f6f-60acf2e0d4a3@efficios.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 1 Mar 2024 11:37:54 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> On 2024-03-01 10:49, Steven Rostedt wrote:
> > On Fri, 1 Mar 2024 13:37:18 +0800
> > linke <lilinke99@qq.com> wrote:
> >   
> >>> So basically you are worried about read-tearing?
> >>>
> >>> That wasn't mentioned in the change log.  
> >>
> >> Yes. Sorry for making this confused, I am not very familiar with this and
> >> still learning.  
> > 
> > No problem. We all have to learn this anyway.
> >   
> >>  
> >>> Funny part is, if the above timestamp read did a tear, then this would
> >>> definitely not match, and would return the correct value. That is, the
> >>> buffer is not empty because the only way for this to get corrupted is if
> >>> something is in the process of writing to it.  
> >>
> >> I agree with you here.
> >>
> >> 	commit = rb_page_commit(commit_page);
> >>
> >> But if commit_page above is the result of a torn read, the commit field
> >> read by rb_page_commit() may not represent a valid value.  
> > 
> > But commit_page is a word length, and I will argue that any compiler that
> > tears "long" words is broken. ;-)  
> 
> [ For those tuning in, we are discussing ring_buffer_iter_empty()
>    "commit_page = cpu_buffer->commit_page;" racy load. ]
> 
> I counter-argue that real-world compilers *are* broken based on your
> personal definition, but we have to deal with them, as documented
> in Documentation/memory-barriers.txt (see below).
> 
> What is the added overhead of using a READ_ONCE() there ? Why are
> we wasting effort trying to guess the compiler behavior if the
> real-world performance impact is insignificant ?
> 
> Quote from memory-barrier.txt explaining the purpose of {READ,WRITE}_ONCE():
> 
> "(*) For aligned memory locations whose size allows them to be accessed
>       with a single memory-reference instruction, prevents "load tearing"
>       and "store tearing," in which a single large access is replaced by
>       multiple smaller accesses."
> 
> I agree that {READ,WRITE}_ONCE() are really not needed at initialization,
> when there are demonstrably no concurrent accesses to the data
> 
> But trying to eliminate {READ,WRITE}_ONCE() on concurrently accessed fields
> just adds complexity, prevents static analyzers to properly understand the
> code and report issues, and just obfuscates the code.
> 
> Thanks,
> 
> Mathieu
> 
> >   
> >>
> >> In this case, READ_ONCE() is only needed for the commit_page.  
> > 
> > But we can at least keep the READ_ONCE() on the commit_page just because it
> > is used in the next instruction.
> 

And here I did state that READ_ONCE() does have another use case. So
there's no argument about adding it here.

-- Steve