Message-Id: <E96297CF-6C81-4BB5-BA5A-6734F6B629F4@gmail.com>
Date:   Thu, 3 Aug 2023 21:36:06 +0800
From:   Alan Huang <mmpgouride@...il.com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org, rcu@...r.kernel.org,
        Will Deacon <will@...nel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Josh Triplett <josh@...htriplett.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Zqiang <qiang.zhang1211@...il.com>,
        Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH 1/2] docs: rcu: Add cautionary note on plain-accesses to
 requirements


> 2023年8月3日 下午8:35，Joel Fernandes <joel@...lfernandes.org> 写道：
> 
> 
> 
>> On Aug 3, 2023, at 8:09 AM, Alan Huang <mmpgouride@...il.com> wrote:
>> 
>> 
>>> 2023年8月3日 11:24，Joel Fernandes (Google) <joel@...lfernandes.org> 写道：
>>> 
>>> Add a detailed note to explain the potential side effects of
>>> plain-accessing the gp pointer using a plain load, without using the
>>> rcu_dereference() macros; which might trip neighboring code that does
>>> use rcu_dereference().
>>> 
>>> I haven't verified this with a compiler, but this is what I gather from
>>> the below link using Will's experience with READ_ONCE().
>>> 
>>> Link: https://lore.kernel.org/all/20230728124412.GA21303@willie-the-truck/
>>> Cc: Will Deacon <will@...nel.org>
>>> Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
>>> ---
>>> .../RCU/Design/Requirements/Requirements.rst  | 32 +++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>> 
>>> diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
>>> index f3b605285a87..e0b896d3fb9b 100644
>>> --- a/Documentation/RCU/Design/Requirements/Requirements.rst
>>> +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
>>> @@ -376,6 +376,38 @@ mechanism, most commonly locking or reference counting
>>> .. |high-quality implementation of C11 memory_order_consume [PDF]| replace:: high-quality implementation of C11 ``memory_order_consume`` [PDF]
>>> .. _high-quality implementation of C11 memory_order_consume [PDF]: http://www.rdrop.com/users/paulmck/RCU/consume.2015.07.13a.pdf
>>> 
>>> +Note that, there can be strange side effects (due to compiler optimizations) if
>>> +``gp`` is ever accessed using a plain load (i.e. without ``READ_ONCE()`` or
>>> +``rcu_dereference()``) potentially hurting any succeeding
>>> +``rcu_dereference()``. For example, consider the code:
>>> +
>>> +   ::
>>> +
>>> +       1 bool do_something_gp(void)
>>> +       2 {
>>> +       3   void *tmp;
>>> +       4   rcu_read_lock();
>>> +       5   tmp = gp; // Plain-load of GP.
>>> +       6   printk("Point gp = %p\n", tmp);
>>> +       7
>>> +       8   p = rcu_dereference(gp);
>>> +       9   if (p) {
>>> +      10     do_something(p->a, p->b);
>>> +      11     rcu_read_unlock();
>>> +      12     return true;
>>> +      13   }
>>> +      14   rcu_read_unlock();
>>> +      15   return false;
>>> +      16 }
>>> +
>>> +The behavior of plain accesses involved in a data race is non-deterministic in
>>> +the face of compiler optimizations. Since accesses to the ``gp`` pointer is
>>> +by-design a data race, the compiler could trip this code by caching the value
>>> +of ``gp`` into a register in line 5, and then using the value of the register
>>> +to satisfy the load in line 10. Thus it is important to never mix
>> 
>> Will’s example is:
>> 
>>    // Assume *ptr is initially 0 and somebody else writes it to 1
>>    // concurrently
>> 
>>    foo = *ptr;
>>    bar = READ_ONCE(*ptr);
>>    baz = *ptr;
>> 
>> Then the compiler is within its right to reorder it to:
>> 
>>   foo = *ptr;
>>   baz = *ptr;
>>   bar = READ_ONCE(*ptr);
>> 
>> So, the result foo == baz == 0 but bar == 1 is perfectly legal.
> 
> Yes, a bad outcome is perfectly legal amidst data race. Who said it is not legal?

My understanding is that it is legal even without data race, and the compiler only keeps the order of volatile access.

> 
>> 
>> But the example here is different,
> 
> That is intentional. Wills discussion partially triggered this. Though I am wondering
> if we should document that as well.
> 
>> the compiler can not use the value loaded from line 5
>> unless the compiler can deduce that the tmp is equals to p in which case the address dependency
>> doesn’t exist anymore.
>> 
>> What am I missing here?
> 
> Maybe you are trying to rationalize too much that the sequence mentioned cannot result
> in a counter intuitive outcome like I did?
> 
> The point AFAIU is not just about line 10 but that the compiler can replace any of the
> lines after the plain access with the cached value.

Well, IIUC, according to the C standard, the compiler can do anything if there is a data race (undefined behavior).

However, what if a write is not protected with WRITE_ONCE and the read is marked with READ_ONCE?
That’s also a data race, right? But the kernel considers it is Okay if the write is machine word aligned.

> 
> Thanks.
> 
> 
> 
>> 
>>> +plain accesses of a memory location with rcu_dereference() of the same memory
>>> +location, in code involved in a data race.
>>> +
>>> In short, updaters use rcu_assign_pointer() and readers use
>>> rcu_dereference(), and these two RCU API elements work together to
>>> ensure that readers have a consistent view of newly added data elements.
>>> -- 
>>> 2.41.0.585.gd2178a4bd4-goog

Powered by blists - more mailing lists