Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

Steven Rostedt
2024-02-25T20:03:05Z

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