lkml.org 
[lkml]   [2021]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: ipc/sem, ipc/msg, ipc/mqueue.c kcsan questions
From
Date
On 5/13/21 9:02 PM, Paul E. McKenney wrote:
> On Thu, May 13, 2021 at 08:10:51AM +0200, Manfred Spraul wrote:
>> Hi Paul,
>>
>> On 5/12/21 10:17 PM, Paul E. McKenney wrote:
>> [...]
>>> int foo;
>>> DEFINE_RWLOCK(foo_rwlock);
>>>
>>> void update_foo(int newval)
>>> {
>>> write_lock(&foo_rwlock);
>>> foo = newval;
>>> do_something(newval);
>>> write_unlock(&foo_rwlock);
>>> }
>>>
>>> int read_foo(void)
>>> {
>>> int ret;
>>>
>>> read_lock(&foo_rwlock);
>>> do_something_else();
>>> ret = foo;
>>> read_unlock(&foo_rwlock);
>>> return ret;
>>> }
>>>
>>> int read_foo_diagnostic(void)
>>> {
>>> return data_race(foo);
>>> }
>> The text didn't help, the example has helped:
>>
>> It was not clear to me if I have to use data_race() both on the read and the
>> write side, or only on one side.
>>
>> Based on this example: plain C may be paired with data_race(), there is no
>> need to mark both sides.
> Actually, you just demonstrated that this example is quite misleading.
> That data_race() works only because the read is for diagnostic
> purposes. I am queuing a commit with your Reported-by that makes
> read_foo_diagnostic() just do a pr_info(), like this:
>
> void read_foo_diagnostic(void)
> {
> pr_info("Current value of foo: %d\n", data_race(foo));
> }
>
> So thank you for that!

I would not like this change at all.
Assume you chase a rare bug, and notice an odd pr_info() output.
It will take you really long until you figure out that a data_race()
mislead you.
Thus for a pr_info(), I would consider READ_ONCE() as the correct thing.

What about something like the attached change?

--

    Manfred


diff --git a/tools/memory-model/Documentation/access-marking.txt b/tools/memory-model/Documentation/access-marking.txt
index 1ab189f51f55..588326b60834 100644
--- a/tools/memory-model/Documentation/access-marking.txt
+++ b/tools/memory-model/Documentation/access-marking.txt
@@ -68,6 +68,11 @@ READ_ONCE() and WRITE_ONCE():

4. Writes setting values that feed into error-tolerant heuristics.

+In theory, plain C-language loads can also be used for these use cases.
+However, in practice this will have the disadvantage of causing KCSAN
+to generate false positives because KCSAN will have no way of knowing
+that the resulting data race was intentional.
+

Data-Racy Reads for Approximate Diagnostics

@@ -86,11 +91,6 @@ that fail to exclude the updates. In this case, it is important to use
data_race() for the diagnostic reads because otherwise KCSAN would give
false-positive warnings about these diagnostic reads.

-In theory, plain C-language loads can also be used for this use case.
-However, in practice this will have the disadvantage of causing KCSAN
-to generate false positives because KCSAN will have no way of knowing
-that the resulting data race was intentional.
-

Data-Racy Reads That Are Checked Against Marked Reload

@@ -110,11 +110,6 @@ that provides the compiler much less scope for mischievous optimizations.
Capturing the return value from cmpxchg() also saves a memory reference
in many cases.

-In theory, plain C-language loads can also be used for this use case.
-However, in practice this will have the disadvantage of causing KCSAN
-to generate false positives because KCSAN will have no way of knowing
-that the resulting data race was intentional.
-

Reads Feeding Into Error-Tolerant Heuristics

@@ -125,11 +120,9 @@ that data_race() loads are subject to load fusing, which can result in
consistent errors, which in turn are quite capable of breaking heuristics.
Therefore use of data_race() should be limited to cases where some other
code (such as a barrier() call) will force the occasional reload.
-
-In theory, plain C-language loads can also be used for this use case.
-However, in practice this will have the disadvantage of causing KCSAN
-to generate false positives because KCSAN will have no way of knowing
-that the resulting data race was intentional.
+The heuristics must be able to handle any error. If the heuristics are
+only able to handle old and new values, then WRITE_ONCE()/READ_ONCE()
+must be used.


Writes Setting Values Feeding Into Error-Tolerant Heuristics
@@ -142,11 +135,8 @@ due to compiler-mangled reads, it can also tolerate the occasional
compiler-mangled write, at least assuming that the proper value is in
place once the write completes.

-Plain C-language stores can also be used for this use case. However,
-in kernels built with CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n, this
-will have the disadvantage of causing KCSAN to generate false positives
-because KCSAN will have no way of knowing that the resulting data race
-was intentional.
+Note that KCSAN will only detect mangled writes in kernels built with
+CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n.


Use of Plain C-Language Accesses
\
 
 \ /
  Last update: 2021-05-14 07:41    [W:0.068 / U:0.820 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site