Message-ID: <a22aa208-19c8-46fa-99aa-10f6ed32d484@efficios.com>
Date: Sat, 28 Sep 2024 11:09:27 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 linux-kernel@...r.kernel.org, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 "Paul E. McKenney" <paulmck@...nel.org>, Will Deacon <will@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Boqun Feng <boqun.feng@...il.com>,
 John Stultz <jstultz@...gle.com>, Neeraj Upadhyay <Neeraj.Upadhyay@....com>,
 Frederic Weisbecker <frederic@...nel.org>,
 Joel Fernandes <joel@...lfernandes.org>,
 Josh Triplett <josh@...htriplett.org>, Uladzislau Rezki <urezki@...il.com>,
 Steven Rostedt <rostedt@...dmis.org>, Lai Jiangshan
 <jiangshanlai@...il.com>, Zqiang <qiang.zhang1211@...il.com>,
 Ingo Molnar <mingo@...hat.com>, Waiman Long <longman@...hat.com>,
 Mark Rutland <mark.rutland@....com>, Thomas Gleixner <tglx@...utronix.de>,
 Vlastimil Babka <vbabka@...e.cz>, maged.michael@...il.com,
 Mateusz Guzik <mjguzik@...il.com>, Gary Guo <gary@...yguo.net>,
 Jonas Oberhauser <jonas.oberhauser@...weicloud.com>, rcu@...r.kernel.org,
 linux-mm@...ck.org, lkmm@...ts.linux.dev
Subject: Re: [PATCH 2/2] Documentation: RCU: Refer to ptr_eq()

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

Powered by blists - more mailing lists