Received-SPF: pass (google.com: domain of linux-kernel+bounces-80263-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 25 Feb 2024 15:03:02 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: linke li <lilinke99@qq.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@efficios.com>, linux-kernel@vger.kernel.org,
 linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH] ring-buffer: use READ_ONCE() to read
 cpu_buffer->commit_page in concurrent environment
Message-ID: <20240225150302.23c3c3c2@rorschach.local.home>
In-Reply-To: <tencent_5EC64EB49686EE61593AE541DB14CE490A08@qq.com>
References: <lilinke99@qq.com>
	<tencent_5EC64EB49686EE61593AE541DB14CE490A08@qq.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 25 Feb 2024 11:05:06 +0800
linke li <lilinke99@qq.com> wrote:

> In function ring_buffer_iter_empty(), cpu_buffer->commit_page and
> curr_commit_page->page->time_stamp is read using READ_ONCE() in 
> line 4354, 4355
> 
> 4354    curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 4355    curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> while they are read directly in line 4340, 4341
> 
> 4340    commit_page = cpu_buffer->commit_page;
> 4341    commit_ts = commit_page->page->time_stamp;

Just because it's used in one place does not mean it's required in
another.

> 
> There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
> This patch find two read of same variable while one is protected, another
> is not. And READ_ONCE() is added to protect.
> 

Here's the entire code:

        cpu_buffer = iter->cpu_buffer;
        reader = cpu_buffer->reader_page;
        head_page = cpu_buffer->head_page;
        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

        /*
         * When the writer goes across pages, it issues a cmpxchg which
         * is a mb(), which will synchronize with the rmb here.
         * (see rb_tail_page_update())
         */
        smp_rmb();

The above smp_rmb() is a full read barrier. The commit_page and
timestamp are not going to be read again after this.

        commit = rb_page_commit(commit_page);
        /* We want to make sure that the commit page doesn't change */
        smp_rmb();

        /* 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);

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.

Thus, the first read of commit_page and time_stamp are read properly as
the compiler will not do anything that can hurt us beyond that
smp_rmb(). The second time we read those variables, we are using them
in the below code.


        /* If the commit page changed, then there's more data */
        if (curr_commit_page != commit_page ||
            curr_commit_ts != commit_ts)
                return 0;

        /* Still racy, as it may return a false positive, but that's OK */
        return ((iter->head_page == commit_page && iter->head >= commit) ||
                (iter->head_page == reader && commit_page == head_page &&
                 head_page->read == commit &&
                 iter->head == rb_page_commit(cpu_buffer->reader_page)));
}

*But* looking at this deeper, the commit_page may need a READ_ONCE()
but not for the reason you suggested.

        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

The commit_page above *is* used again, and we want commit_ts to be part
of the commit_page that was originally read and not a second reading.

So, I think for the commit_page we do need a READ_ONCE() but that's
because it is referenced again just below it and we don't want the
compiler to read the memory location again for the second reference.

-- Steve