On Sun, 25 Feb 2024 11:05:06 +0800 linke li <lilink...@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