On Thu, Jul 03, 2025 at 06:54:02PM -0700, Shakeel Butt wrote: > On Thu, Jul 3, 2025 at 4:53 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote: > > > > On Thu, Jul 03, 2025 at 03:46:07PM -0700, Shakeel Butt wrote: > [...] > > > Let me answer this one first. The previous patch actually made > > > init_llist_node() do WRITE_ONCE(). > > > > > > So the actual question is why do we need > > > data_race([READ|WRITE]_ONCE()) instead of just [READ|WRITE]_ONCE()? > > > > You should *almost* always use [READ|WRITE]_ONCE() instead of data_race(). > > > > > Actually I had the similar question myself and found the following > > > comment in include/linux/compiler.h: > > > > > > /** > > > * data_race - mark an expression as containing intentional data races > > > * > > > * This data_race() macro is useful for situations in which data races > > > * should be forgiven. One example is diagnostic code that accesses > > > * shared variables but is not a part of the core synchronization design. > > > * For example, if accesses to a given variable are protected by a lock, > > > * except for diagnostic code, then the accesses under the lock should > > > * be plain C-language accesses and those in the diagnostic code should > > > * use data_race(). This way, KCSAN will complain if buggy lockless > > > * accesses to that variable are introduced, even if the buggy accesses > > > * are protected by READ_ONCE() or WRITE_ONCE(). > > > * > > > * This macro *does not* affect normal code generation, but is a hint > > > * to tooling that data races here are to be ignored. If the access must > > > * be atomic *and* KCSAN should ignore the access, use both data_race() > > > * and READ_ONCE(), for example, data_race(READ_ONCE(x)). > > > */ > > > > > > IIUC correctly, I need to protect llist_node against tearing and as well > > > as tell KCSAN to ignore the access for race then I should use both. > > > Though I think KCSAN treat [READ|WRITE]_ONCE similar to data_race(), so > > > it kind of seem redundant but I think at least I want to convey that we > > > need protection against tearing and ignore KCSAN and using both conveys > > > that. Let me know if you think otherwise. > > > > > > thanks a lot for taking a look. > > > > The thing to remember is that data_race() does not affect the > > generated code (except of course when running KCSAN), and thus does > > absolutely nothing to prevent load/store tearing. You need things like > > [READ|WRITE]_ONCE() to prevent tearing. > > > > So if it does not affect the generated code, what is the point of > > data_race()? > > > > One answer to this question is for diagnostics where you want KCSAN > > to check the main algorithm, but you don't want KCSAN to be confused > > by the diagnostic accesses. For example, you might use something like > > ASSERT_EXCLUSIVE_ACCESS() as in __list_splice_init_rcu(), and not want > > your diagnostic accesses to result in false-positive KCSAN reports > > due to interactions with ASSERT_EXCLUSIVE_ACCESS() on some particular > > memory location. And if you were to use READ_ONCE() to access that same > > memory location in your diagnostics, KCSAN would complain if they ran > > concurrently with that ASSERT_EXCLUSIVE_ACCESS(). So you would instead > > use data_race() to suppress such complaints. > > > > Does that make sense? > > Thanks a lot Paul for the awesome explanation. Do you think keeping > data_race() here would be harmful in a sense that it might cause > confusion in future? Yes, plus it might incorrectly suppress a KCSAN warning for a very real bug. So I strongly recommend removing the data_race() in this case. Thanx, Paul
