Hello Miklos,
On Fri, May 10, 2024 at 11:21:19AM +0200, Miklos Szeredi wrote:
> On Thu, 9 May 2024 at 14:57, Breno Leitao <leitao@xxxxxxxxxx> wrote:
>
> > Annotated the reader with READ_ONCE() and the writer with WRITE_ONCE()
> > to avoid such complaint from KCSAN.
>
> I'm not sure the write side part is really needed, since the lock is
> properly protecting against concurrent readers/writers within the
> locked region.
I understand that num_background is read from an unlocked region
(fuse_readahead()).
> Does KCSAN still complain if you just add the READ_ONCE() to fuse_readahead()?
I haven't checked, but, looking at the documentation it says that both part
needs to be marked. Here is an example very similar to ours here, from
tools/memory-model/Documentation/access-marking.txt
Lock-Protected Writes With Lockless Reads
-----------------------------------------
For another example, suppose a shared variable "foo" is updated only
while holding a spinlock, but is read locklessly. The code might look
as follows:
int foo;
DEFINE_SPINLOCK(foo_lock);
void update_foo(int newval)
{
spin_lock(&foo_lock);
WRITE_ONCE(foo, newval);
ASSERT_EXCLUSIVE_WRITER(foo);
do_something(newval);
spin_unlock(&foo_wlock);
}
int read_foo(void)
{
do_something_else();
return READ_ONCE(foo);
}
Because foo is read locklessly, all accesses are marked.
>From my understanding, we need a WRITE_ONCE() inside the lock, because
the bg_lock lock in fuse_request_end() is invisible for fuse_readahead(),
and fuse_readahead() might read num_backgroud that was writen
non-atomically/corrupted (if there is no WRITE_ONCE()).
That said, if the reader (fuse_readahead()) can handle possible
corrupted data, we can mark is with data_race() annotation. Then I
understand we don't need to mark the write with WRITE_ONCE().
Here is what access-marking.txt says about this case:
Here are some situations where data_race() should be used instead of
READ_ONCE() and WRITE_ONCE():
1. Data-racy loads from shared variables whose values are used only
for diagnostic purposes.
2. Data-racy reads whose values are checked against marked reload.
3. Reads whose values feed into error-tolerant heuristics.
4. Writes setting values that feed into error-tolerant heuristics.
Anyway, I am more than happy to test with only a READ_ONLY() in the
reader side, if that the approach you prefer.
Thanks!
