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

[Mathieu Desnoyers]

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