Re: [PATCH 2/2] Documentation: RCU: Refer to ptr_eq()

Mathieu Desnoyers
2024-09-28T15:11:31Z

On 2024-09-28 16:58, Alan Stern wrote:
> On Sat, Sep 28, 2024 at 09:51:28AM -0400, Mathieu Desnoyers wrote:
[...]
>> -- Be very careful about comparing pointers obtained from
>> - rcu_dereference() against non-NULL values. As Linus Torvalds
>> - explained, if the two pointers are equal, the compiler could
>> - substitute the pointer you are comparing against for the pointer
>> - obtained from rcu_dereference(). For example::
>> +- Use relational operators which preserve address dependencies
>> + (such as "ptr_eq()") to compare pointers obtained from
>
> Nit: ptr_eq() is an inline function, not a relational operator. Say
> "operations that" instead of "relational operators which".
>
>> + rcu_dereference() against non-NULL values or against pointers

Note: here I need to update the wording as well:

+- Use operations that preserve address dependencies (such as
+ "ptr_eq()") to compare pointers obtained from rcu_dereference()
+ against non-NULL pointers. As Linus Torvalds explained, if the
+ two pointers are equal, the compiler could substitute the
+ pointer you are comparing against for the pointer obtained from
+ rcu_dereference(). For example::

>> + obtained from prior loads. As Linus Torvalds explained, if the
>> + two pointers are equal, the compiler could substitute the
>> + pointer you are comparing against for the pointer obtained from
>> + rcu_dereference(). For example::
>>
>> p = rcu_dereference(gp);
>> if (p == &default_struct)
>> @@ -125,6 +127,23 @@ readers working properly:
>> On ARM and Power hardware, the load from "default_struct.a"
>> can now be speculated, such that it might happen before the
>> rcu_dereference(). This could result in bugs due to misordering.
>> + Performing the comparison with "ptr_eq()" ensures the compiler
>> + does not perform such transformation.
>> +
>> + If the comparison is against a pointer obtained from prior
>> + loads, the compiler is allowed to use either register for the
>
> This is true even when the comparison is against a pointer obtained from
> a later load. Just say "another pointer" instead of "a pointer obtained
> from prior loads". (And why would someone need multiple loads to
> obtain a single pointer?)
>
> Also, say "pointer" instead of "register".

OK.

>
>> + following accesses, which loses the address dependency and
>> + allows weakly-ordered architectures such as ARM and PowerPC
>> + to speculate the address-dependent load before rcu_dereference().
>> + For example::
>> +
>> + p1 = READ_ONCE(gp);
>> + p2 = rcu_dereference(gp);
>> + if (p1 == p2)
>> + do_default(p2->a);
>
> Here you should say that the compiler could use p1->a rather than p2->a,
> destroying the address dependency. That's the whole point of this; you
> shouldn't skip over it.

OK.

>
>> +
>> + Performing the comparison with "ptr_eq()" ensures the compiler
>> + preserves the address dependencies.
>>
>> However, comparisons are OK in the following cases:
>>
>> @@ -204,6 +223,11 @@ readers working properly:
>> comparison will provide exactly the information that the
>> compiler needs to deduce the value of the pointer.
>>
>> + When in doubt, use relational operators that preserve address
>
> Again, "operations" instead of "relational operators".

OK. Will fix in my next round.

Thanks,

Mathieu

>
> Alan Stern
>
>> + dependencies (such as "ptr_eq()") to compare pointers obtained
>> + from rcu_dereference() against non-NULL values or against
>> + pointers obtained from prior loads.
>> +
>> - Disable any value-speculation optimizations that your compiler
>> might provide, especially if you are making use of feedback-based
>> optimizations that take data collected from prior runs. Such
>> --
>> 2.39.2
>>

--
Mathieu Desnoyers
EfficiOS Inc.