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.
https://www.efficios.com


Reply via email to