On Fri, 1 Mar 2024 11:37:54 -0500 Mathieu Desnoyers <mathieu.desnoy...@efficios.com> wrote:
> On 2024-03-01 10:49, Steven Rostedt wrote: > > On Fri, 1 Mar 2024 13:37:18 +0800 > > linke <lilink...@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