[PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[Andrea Parri]

Quoting Paul [1]:

 "Given that a quick (and perhaps error-prone) search of the uses
  of rcu_assign_pointer() in v5.1 didn't find a single use of the
  return value, let's please instead change the documentation and
  implementation to eliminate the return value."

[1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com

Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: rcu@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sasha Levin <sashal@kernel.org>
---
Matthew, Sasha:

The patch is based on -rcu/dev; I took the liberty of applying the
same change to your #defines in:

 tools/testing/radix-tree/linux/rcupdate.h
 tools/include/linux/rcu.h

but I admit that I'm not familiar with their uses: please shout if
you have any objections with it.
---
 Documentation/RCU/whatisRCU.txt           |  8 ++++----
 include/linux/rcupdate.h                  |  5 ++---
 tools/include/linux/rcu.h                 | 11 +++++++++--
 tools/testing/radix-tree/linux/rcupdate.h |  5 ++++-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 981651a8b65d2..f99a87b9a88fa 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -212,7 +212,7 @@ synchronize_rcu()
 
 rcu_assign_pointer()
 
-	typeof(p) rcu_assign_pointer(p, typeof(p) v);
+	rcu_assign_pointer(p, typeof(p) v);
 
 	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
 	would be cool to be able to declare a function in this manner.
@@ -220,9 +220,9 @@ rcu_assign_pointer()
 
 	The updater uses this function to assign a new value to an
 	RCU-protected pointer, in order to safely communicate the change
-	in value from the updater to the reader.  This function returns
-	the new value, and also executes any memory-barrier instructions
-	required for a given CPU architecture.
+	in value from the updater to the reader.  This macro does not
+	evaluate to an rvalue, but it does execute any memory-barrier
+	instructions required for a given CPU architecture.
 
 	Perhaps just as important, it serves to document (1) which
 	pointers are protected by RCU and (2) the point at which a
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 915460ec08722..a5f61a08e65fc 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
  * other macros that it invokes.
  */
 #define rcu_assign_pointer(p, v)					      \
-({									      \
+do {									      \
 	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
 	rcu_check_sparse(p, __rcu);				      \
 									      \
@@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
 	else								      \
 		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
-	_r_a_p__v;							      \
-})
+} while (0)
 
 /**
  * rcu_swap_protected() - swap an RCU and a regular pointer
diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
index 7d02527e5bcea..01a435ee48cd6 100644
--- a/tools/include/linux/rcu.h
+++ b/tools/include/linux/rcu.h
@@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
 	return false;
 }
 
-#define rcu_assign_pointer(p, v) ((p) = (v))
-#define RCU_INIT_POINTER(p, v) p=(v)
+#define rcu_assign_pointer(p, v)				\
+do {								\
+	(p) = (v);						\
+} while (0)
+
+#define RCU_INIT_POINTER(p, v)					\
+do {								\
+	(p) = (v);						\
+} while (0)
 
 #endif
diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
index fd280b070fdb1..48212f3a758e6 100644
--- a/tools/testing/radix-tree/linux/rcupdate.h
+++ b/tools/testing/radix-tree/linux/rcupdate.h
@@ -7,6 +7,9 @@
 #define rcu_dereference_raw(p) rcu_dereference(p)
 #define rcu_dereference_protected(p, cond) rcu_dereference(p)
 #define rcu_dereference_check(p, cond) rcu_dereference(p)
-#define RCU_INIT_POINTER(p, v)	(p) = (v)
+#define RCU_INIT_POINTER(p, v)					\
+do {								\
+	(p) = (v);						\
+} while (0)
 
 #endif
-- 
2.7.4

Re: [PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[Paul E. McKenney]

On Fri, May 24, 2019 at 12:36:37PM +0200, Andrea Parri wrote:
> Quoting Paul [1]:
> 
>  "Given that a quick (and perhaps error-prone) search of the uses
>   of rcu_assign_pointer() in v5.1 didn't find a single use of the
>   return value, let's please instead change the documentation and
>   implementation to eliminate the return value."
> 
> [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com

Thank you!  A few comments below.

							Thanx, Paul

> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: rcu@vger.kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Sasha Levin <sashal@kernel.org>
> ---
> Matthew, Sasha:
> 
> The patch is based on -rcu/dev; I took the liberty of applying the
> same change to your #defines in:
> 
>  tools/testing/radix-tree/linux/rcupdate.h
>  tools/include/linux/rcu.h
> 
> but I admit that I'm not familiar with their uses: please shout if
> you have any objections with it.
> ---
>  Documentation/RCU/whatisRCU.txt           |  8 ++++----
>  include/linux/rcupdate.h                  |  5 ++---
>  tools/include/linux/rcu.h                 | 11 +++++++++--
>  tools/testing/radix-tree/linux/rcupdate.h |  5 ++++-
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> index 981651a8b65d2..f99a87b9a88fa 100644
> --- a/Documentation/RCU/whatisRCU.txt
> +++ b/Documentation/RCU/whatisRCU.txt
> @@ -212,7 +212,7 @@ synchronize_rcu()
>  
>  rcu_assign_pointer()
>  
> -	typeof(p) rcu_assign_pointer(p, typeof(p) v);
> +	rcu_assign_pointer(p, typeof(p) v);

Please add the "void", similar to synchronize_rcu() above.

>  	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
>  	would be cool to be able to declare a function in this manner.
> @@ -220,9 +220,9 @@ rcu_assign_pointer()
>  
>  	The updater uses this function to assign a new value to an
>  	RCU-protected pointer, in order to safely communicate the change
> -	in value from the updater to the reader.  This function returns
> -	the new value, and also executes any memory-barrier instructions
> -	required for a given CPU architecture.
> +	in value from the updater to the reader.  This macro does not
> +	evaluate to an rvalue, but it does execute any memory-barrier
> +	instructions required for a given CPU architecture.
>  
>  	Perhaps just as important, it serves to document (1) which
>  	pointers are protected by RCU and (2) the point at which a
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 915460ec08722..a5f61a08e65fc 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>   * other macros that it invokes.
>   */
>  #define rcu_assign_pointer(p, v)					      \
> -({									      \
> +do {									      \
>  	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
>  	rcu_check_sparse(p, __rcu);				      \
>  									      \
> @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>  		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
>  	else								      \
>  		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> -	_r_a_p__v;							      \
> -})
> +} while (0)
>  
>  /**
>   * rcu_swap_protected() - swap an RCU and a regular pointer
> diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> index 7d02527e5bcea..01a435ee48cd6 100644
> --- a/tools/include/linux/rcu.h
> +++ b/tools/include/linux/rcu.h
> @@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
>  	return false;
>  }
>  
> -#define rcu_assign_pointer(p, v) ((p) = (v))
> -#define RCU_INIT_POINTER(p, v) p=(v)
> +#define rcu_assign_pointer(p, v)				\
> +do {								\
> +	(p) = (v);						\
> +} while (0)
> +
> +#define RCU_INIT_POINTER(p, v)					\
> +do {								\
> +	(p) = (v);						\
> +} while (0)

These two each fit nicely on one line:

	#define rcu_assign_pointer(p, v) do { (p) = (v); } while (0)
	#define RCU_INIT_POINTER(p, v)   do { (p) = (v); } while (0)

>  
>  #endif
> diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> index fd280b070fdb1..48212f3a758e6 100644
> --- a/tools/testing/radix-tree/linux/rcupdate.h
> +++ b/tools/testing/radix-tree/linux/rcupdate.h
> @@ -7,6 +7,9 @@
>  #define rcu_dereference_raw(p) rcu_dereference(p)
>  #define rcu_dereference_protected(p, cond) rcu_dereference(p)
>  #define rcu_dereference_check(p, cond) rcu_dereference(p)
> -#define RCU_INIT_POINTER(p, v)	(p) = (v)
> +#define RCU_INIT_POINTER(p, v)					\
> +do {								\
> +	(p) = (v);						\
> +} while (0)

As does this one.

>  #endif
> -- 
> 2.7.4
> 

Re: [PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[Joel Fernandes]

On Fri, May 24, 2019 at 12:36:37PM +0200, Andrea Parri wrote:
> Quoting Paul [1]:
> 
>  "Given that a quick (and perhaps error-prone) search of the uses
>   of rcu_assign_pointer() in v5.1 didn't find a single use of the
>   return value, let's please instead change the documentation and
>   implementation to eliminate the return value."
> 
> [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com
> 
> Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: rcu@vger.kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Sasha Levin <sashal@kernel.org>
> ---
> Matthew, Sasha:
> 
> The patch is based on -rcu/dev; I took the liberty of applying the
> same change to your #defines in:
> 
>  tools/testing/radix-tree/linux/rcupdate.h
>  tools/include/linux/rcu.h
> 
> but I admit that I'm not familiar with their uses: please shout if
> you have any objections with it.
> ---
>  Documentation/RCU/whatisRCU.txt           |  8 ++++----
>  include/linux/rcupdate.h                  |  5 ++---
>  tools/include/linux/rcu.h                 | 11 +++++++++--
>  tools/testing/radix-tree/linux/rcupdate.h |  5 ++++-
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> index 981651a8b65d2..f99a87b9a88fa 100644
> --- a/Documentation/RCU/whatisRCU.txt
> +++ b/Documentation/RCU/whatisRCU.txt
> @@ -212,7 +212,7 @@ synchronize_rcu()
>  
>  rcu_assign_pointer()
>  
> -	typeof(p) rcu_assign_pointer(p, typeof(p) v);
> +	rcu_assign_pointer(p, typeof(p) v);
>  
>  	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
>  	would be cool to be able to declare a function in this manner.
> @@ -220,9 +220,9 @@ rcu_assign_pointer()
>  
>  	The updater uses this function to assign a new value to an
>  	RCU-protected pointer, in order to safely communicate the change
> -	in value from the updater to the reader.  This function returns
> -	the new value, and also executes any memory-barrier instructions
> -	required for a given CPU architecture.
> +	in value from the updater to the reader.  This macro does not
> +	evaluate to an rvalue, but it does execute any memory-barrier
> +	instructions required for a given CPU architecture.
>  
>  	Perhaps just as important, it serves to document (1) which
>  	pointers are protected by RCU and (2) the point at which a
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 915460ec08722..a5f61a08e65fc 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>   * other macros that it invokes.
>   */
>  #define rcu_assign_pointer(p, v)					      \
> -({									      \
> +do {									      \
>  	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
>  	rcu_check_sparse(p, __rcu);				      \
>  									      \
> @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>  		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
>  	else								      \
>  		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> -	_r_a_p__v;							      \
> -})
> +} while (0)
>  
>  /**
>   * rcu_swap_protected() - swap an RCU and a regular pointer
> diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> index 7d02527e5bcea..01a435ee48cd6 100644
> --- a/tools/include/linux/rcu.h
> +++ b/tools/include/linux/rcu.h
> @@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
>  	return false;
>  }
>  
> -#define rcu_assign_pointer(p, v) ((p) = (v))
> -#define RCU_INIT_POINTER(p, v) p=(v)
> +#define rcu_assign_pointer(p, v)				\
> +do {								\
> +	(p) = (v);						\
> +} while (0)
> +
> +#define RCU_INIT_POINTER(p, v)					\
> +do {								\
> +	(p) = (v);						\
> +} while (0)
>  
>  #endif
> diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> index fd280b070fdb1..48212f3a758e6 100644
> --- a/tools/testing/radix-tree/linux/rcupdate.h
> +++ b/tools/testing/radix-tree/linux/rcupdate.h
> @@ -7,6 +7,9 @@
>  #define rcu_dereference_raw(p) rcu_dereference(p)
>  #define rcu_dereference_protected(p, cond) rcu_dereference(p)
>  #define rcu_dereference_check(p, cond) rcu_dereference(p)
> -#define RCU_INIT_POINTER(p, v)	(p) = (v)
> +#define RCU_INIT_POINTER(p, v)					\
> +do {								\
> +	(p) = (v);						\
> +} while (0)
>  
>  #endif
> -- 
> 2.7.4
> 

Other than Paul's nits, LGTM. Thanks.

Re: [PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[Andrea Parri]

On Fri, May 24, 2019 at 06:29:11AM -0700, Paul E. McKenney wrote:
> On Fri, May 24, 2019 at 12:36:37PM +0200, Andrea Parri wrote:
> > Quoting Paul [1]:
> > 
> >  "Given that a quick (and perhaps error-prone) search of the uses
> >   of rcu_assign_pointer() in v5.1 didn't find a single use of the
> >   return value, let's please instead change the documentation and
> >   implementation to eliminate the return value."
> > 
> > [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com
> 
> Thank you!  A few comments below.

Thank you for the suggestions, Paul.


> 
> 							Thanx, Paul
> 
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > Cc: rcu@vger.kernel.org
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Sasha Levin <sashal@kernel.org>
> > ---
> > Matthew, Sasha:
> > 
> > The patch is based on -rcu/dev; I took the liberty of applying the
> > same change to your #defines in:
> > 
> >  tools/testing/radix-tree/linux/rcupdate.h
> >  tools/include/linux/rcu.h
> > 
> > but I admit that I'm not familiar with their uses: please shout if
> > you have any objections with it.
> > ---
> >  Documentation/RCU/whatisRCU.txt           |  8 ++++----
> >  include/linux/rcupdate.h                  |  5 ++---
> >  tools/include/linux/rcu.h                 | 11 +++++++++--
> >  tools/testing/radix-tree/linux/rcupdate.h |  5 ++++-
> >  4 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> > index 981651a8b65d2..f99a87b9a88fa 100644
> > --- a/Documentation/RCU/whatisRCU.txt
> > +++ b/Documentation/RCU/whatisRCU.txt
> > @@ -212,7 +212,7 @@ synchronize_rcu()
> >  
> >  rcu_assign_pointer()
> >  
> > -	typeof(p) rcu_assign_pointer(p, typeof(p) v);
> > +	rcu_assign_pointer(p, typeof(p) v);
> 
> Please add the "void", similar to synchronize_rcu() above.

Sure, will do in v2.


> 
> >  	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
> >  	would be cool to be able to declare a function in this manner.
> > @@ -220,9 +220,9 @@ rcu_assign_pointer()
> >  
> >  	The updater uses this function to assign a new value to an
> >  	RCU-protected pointer, in order to safely communicate the change
> > -	in value from the updater to the reader.  This function returns
> > -	the new value, and also executes any memory-barrier instructions
> > -	required for a given CPU architecture.
> > +	in value from the updater to the reader.  This macro does not
> > +	evaluate to an rvalue, but it does execute any memory-barrier
> > +	instructions required for a given CPU architecture.
> >  
> >  	Perhaps just as important, it serves to document (1) which
> >  	pointers are protected by RCU and (2) the point at which a
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 915460ec08722..a5f61a08e65fc 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> >   * other macros that it invokes.
> >   */
> >  #define rcu_assign_pointer(p, v)					      \
> > -({									      \
> > +do {									      \
> >  	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
> >  	rcu_check_sparse(p, __rcu);				      \
> >  									      \
> > @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> >  		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
> >  	else								      \
> >  		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> > -	_r_a_p__v;							      \
> > -})
> > +} while (0)
> >  
> >  /**
> >   * rcu_swap_protected() - swap an RCU and a regular pointer
> > diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> > index 7d02527e5bcea..01a435ee48cd6 100644
> > --- a/tools/include/linux/rcu.h
> > +++ b/tools/include/linux/rcu.h
> > @@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
> >  	return false;
> >  }
> >  
> > -#define rcu_assign_pointer(p, v) ((p) = (v))
> > -#define RCU_INIT_POINTER(p, v) p=(v)
> > +#define rcu_assign_pointer(p, v)				\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> > +
> > +#define RCU_INIT_POINTER(p, v)					\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> 
> These two each fit nicely on one line:
> 
> 	#define rcu_assign_pointer(p, v) do { (p) = (v); } while (0)
> 	#define RCU_INIT_POINTER(p, v)   do { (p) = (v); } while (0)

Same here.


> 
> >  
> >  #endif
> > diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> > index fd280b070fdb1..48212f3a758e6 100644
> > --- a/tools/testing/radix-tree/linux/rcupdate.h
> > +++ b/tools/testing/radix-tree/linux/rcupdate.h
> > @@ -7,6 +7,9 @@
> >  #define rcu_dereference_raw(p) rcu_dereference(p)
> >  #define rcu_dereference_protected(p, cond) rcu_dereference(p)
> >  #define rcu_dereference_check(p, cond) rcu_dereference(p)
> > -#define RCU_INIT_POINTER(p, v)	(p) = (v)
> > +#define RCU_INIT_POINTER(p, v)					\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> 
> As does this one.

... And here.

Thanks,
  Andrea

Re: [PATCH] rcu: Prevent evaluation of rcu_assign_pointer()

[Andrea Parri]

On Fri, May 24, 2019 at 12:45:09PM -0400, Joel Fernandes wrote:
> On Fri, May 24, 2019 at 12:36:37PM +0200, Andrea Parri wrote:
> > Quoting Paul [1]:
> > 
> >  "Given that a quick (and perhaps error-prone) search of the uses
> >   of rcu_assign_pointer() in v5.1 didn't find a single use of the
> >   return value, let's please instead change the documentation and
> >   implementation to eliminate the return value."
> > 
> > [1] https://lkml.kernel.org/r/20190523135013.GL28207@linux.ibm.com
> > 
> > Signed-off-by: Andrea Parri <andrea.parri@amarulasolutions.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > Cc: rcu@vger.kernel.org
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Sasha Levin <sashal@kernel.org>
> > ---
> > Matthew, Sasha:
> > 
> > The patch is based on -rcu/dev; I took the liberty of applying the
> > same change to your #defines in:
> > 
> >  tools/testing/radix-tree/linux/rcupdate.h
> >  tools/include/linux/rcu.h
> > 
> > but I admit that I'm not familiar with their uses: please shout if
> > you have any objections with it.
> > ---
> >  Documentation/RCU/whatisRCU.txt           |  8 ++++----
> >  include/linux/rcupdate.h                  |  5 ++---
> >  tools/include/linux/rcu.h                 | 11 +++++++++--
> >  tools/testing/radix-tree/linux/rcupdate.h |  5 ++++-
> >  4 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> > index 981651a8b65d2..f99a87b9a88fa 100644
> > --- a/Documentation/RCU/whatisRCU.txt
> > +++ b/Documentation/RCU/whatisRCU.txt
> > @@ -212,7 +212,7 @@ synchronize_rcu()
> >  
> >  rcu_assign_pointer()
> >  
> > -	typeof(p) rcu_assign_pointer(p, typeof(p) v);
> > +	rcu_assign_pointer(p, typeof(p) v);
> >  
> >  	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
> >  	would be cool to be able to declare a function in this manner.
> > @@ -220,9 +220,9 @@ rcu_assign_pointer()
> >  
> >  	The updater uses this function to assign a new value to an
> >  	RCU-protected pointer, in order to safely communicate the change
> > -	in value from the updater to the reader.  This function returns
> > -	the new value, and also executes any memory-barrier instructions
> > -	required for a given CPU architecture.
> > +	in value from the updater to the reader.  This macro does not
> > +	evaluate to an rvalue, but it does execute any memory-barrier
> > +	instructions required for a given CPU architecture.
> >  
> >  	Perhaps just as important, it serves to document (1) which
> >  	pointers are protected by RCU and (2) the point at which a
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 915460ec08722..a5f61a08e65fc 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -367,7 +367,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> >   * other macros that it invokes.
> >   */
> >  #define rcu_assign_pointer(p, v)					      \
> > -({									      \
> > +do {									      \
> >  	uintptr_t _r_a_p__v = (uintptr_t)(v);				      \
> >  	rcu_check_sparse(p, __rcu);				      \
> >  									      \
> > @@ -375,8 +375,7 @@ static inline void rcu_preempt_sleep_check(void) { }
> >  		WRITE_ONCE((p), (typeof(p))(_r_a_p__v));		      \
> >  	else								      \
> >  		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> > -	_r_a_p__v;							      \
> > -})
> > +} while (0)
> >  
> >  /**
> >   * rcu_swap_protected() - swap an RCU and a regular pointer
> > diff --git a/tools/include/linux/rcu.h b/tools/include/linux/rcu.h
> > index 7d02527e5bcea..01a435ee48cd6 100644
> > --- a/tools/include/linux/rcu.h
> > +++ b/tools/include/linux/rcu.h
> > @@ -19,7 +19,14 @@ static inline bool rcu_is_watching(void)
> >  	return false;
> >  }
> >  
> > -#define rcu_assign_pointer(p, v) ((p) = (v))
> > -#define RCU_INIT_POINTER(p, v) p=(v)
> > +#define rcu_assign_pointer(p, v)				\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> > +
> > +#define RCU_INIT_POINTER(p, v)					\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> >  
> >  #endif
> > diff --git a/tools/testing/radix-tree/linux/rcupdate.h b/tools/testing/radix-tree/linux/rcupdate.h
> > index fd280b070fdb1..48212f3a758e6 100644
> > --- a/tools/testing/radix-tree/linux/rcupdate.h
> > +++ b/tools/testing/radix-tree/linux/rcupdate.h
> > @@ -7,6 +7,9 @@
> >  #define rcu_dereference_raw(p) rcu_dereference(p)
> >  #define rcu_dereference_protected(p, cond) rcu_dereference(p)
> >  #define rcu_dereference_check(p, cond) rcu_dereference(p)
> > -#define RCU_INIT_POINTER(p, v)	(p) = (v)
> > +#define RCU_INIT_POINTER(p, v)					\
> > +do {								\
> > +	(p) = (v);						\
> > +} while (0)
> >  
> >  #endif
> > -- 
> > 2.7.4
> > 
> 
> Other than Paul's nits, LGTM. Thanks.

Thank you, Joel!  Will fix those and resend, most likely next week.

  Andrea