Received-SPF: pass (google.com: domain of linux-kernel+bounces-86913-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 29 Feb 2024 10:21:19 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: linke <lilinke99@qq.com>
Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
 mathieu.desnoyers@efficios.com, mhiramat@kernel.org
Subject: Re: [PATCH] ring-buffer: use READ_ONCE() to read
 cpu_buffer->commit_page in concurrent environment
Message-ID: <20240229102119.7c475dee@gandalf.local.home>
In-Reply-To: <tencent_BE8C3169030CD9C2FC7548832C2994921609@qq.com>
References: <tencent_BE8C3169030CD9C2FC7548832C2994921609@qq.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 29 Feb 2024 20:32:26 +0800
linke <lilinke99@qq.com> wrote:

> Hi Steven, sorry for the late reply.
> 
> > 
> > Now the reason for the above READ_ONCE() is because the variables *are*
> > going to be used again. We do *not* want the compiler to play any games
> > with that.
> >   
> 
> I don't think it is because the variables are going to be used again. 
> Compiler optimizations barely do bad things in single thread programs. It
> is because cpu_buffer->commit_page may change concurrently and should be
> accessed atomically.

So basically you are worried about read-tearing?

That wasn't mentioned in the change log.

> 
> 	/* Make sure commit page didn't change */
> 	curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 	curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> 	/* If the commit page changed, then there's more data */
> 	if (curr_commit_page != commit_page ||
> 	    curr_commit_ts != commit_ts)
> 		return 0;
> 
> This code read cpu_buffer->commit_page and time_stamp again to check
> whether commit page changed. It shows that cpu_buffer->commit_page and 
> time_stamp may be changed by other threads.
> 
>         commit_page = cpu_buffer->commit_page;
>         commit_ts = commit_page->page->time_stamp;
> 
> So the commit_page and time_stamp above is read while other threads may
> change it. I think it is a data race if it is not atomic. Thus it is 
> necessary to use READ_ONCE() here.

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.

Now we could add a comment stating this.

So, I don't even think the reading of the commit_page is needed (it's long
size so it should not tear, and if it does, I consider that more a bug in
the compiler).

Please explain why READ_ONCE() is needed, and what exactly is it "fixing".
That is, what breaks if it's not there?

-- Steve