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/[email protected]
Signed-off-by: Andrea Parri <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Sasha Levin <[email protected]>
---
Changes in v2:
- Fix documentation and style (Paul E. McKenney)
- Improve subject line (Mark Rutland)
---
Documentation/RCU/whatisRCU.txt | 8 ++++----
include/linux/rcupdate.h | 5 ++---
tools/include/linux/rcu.h | 4 ++--
tools/testing/radix-tree/linux/rcupdate.h | 2 +-
4 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 981651a8b65d2..7e1a8721637ab 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);
+ void 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 a8ed624da5556..0c9b92799abc7 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..9554d3fa54f33 100644
--- a/tools/include/linux/rcu.h
+++ b/tools/include/linux/rcu.h
@@ -19,7 +19,7 @@ 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..fed468fb0c78d 100644
--- a/tools/testing/radix-tree/linux/rcupdate.h
+++ b/tools/testing/radix-tree/linux/rcupdate.h
@@ -7,6 +7,6 @@
#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
On Mon, May 27, 2019 at 10:49:57AM +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/[email protected]
Queued, thank you!
Adding the checkpatch maintainers on CC as well. The "do { } while
(0)" prevents the return value from being used, by design. Given the
checkpatch complaint, is there some better way to achieve this?
Thanx, Paul
> Signed-off-by: Andrea Parri <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: [email protected]
> Cc: Peter Zijlstra <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Sasha Levin <[email protected]>
> ---
> Changes in v2:
> - Fix documentation and style (Paul E. McKenney)
> - Improve subject line (Mark Rutland)
> ---
> Documentation/RCU/whatisRCU.txt | 8 ++++----
> include/linux/rcupdate.h | 5 ++---
> tools/include/linux/rcu.h | 4 ++--
> tools/testing/radix-tree/linux/rcupdate.h | 2 +-
> 4 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
> index 981651a8b65d2..7e1a8721637ab 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);
> + void 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 a8ed624da5556..0c9b92799abc7 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..9554d3fa54f33 100644
> --- a/tools/include/linux/rcu.h
> +++ b/tools/include/linux/rcu.h
> @@ -19,7 +19,7 @@ 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..fed468fb0c78d 100644
> --- a/tools/testing/radix-tree/linux/rcupdate.h
> +++ b/tools/testing/radix-tree/linux/rcupdate.h
> @@ -7,6 +7,6 @@
> #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
>
On Mon, 2019-05-27 at 09:10 -0700, Paul E. McKenney wrote:
> On Mon, May 27, 2019 at 10:49:57AM +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/[email protected]
>
> Queued, thank you!
>
> Adding the checkpatch maintainers on CC as well. The "do { } while
> (0)" prevents the return value from being used, by design. Given the
> checkpatch complaint, is there some better way to achieve this?
Not sure what the checkpatch complaint is here.
Reading the link above, there seems to be a compiler warning.
Perhaps a statement expression macro with no return value?
#define rcu_assign_pointer(p, v) ({ (p) = (v); ; })
On Mon, May 27, 2019 at 10:21:22AM -0700, Joe Perches wrote:
> On Mon, 2019-05-27 at 09:10 -0700, Paul E. McKenney wrote:
> > On Mon, May 27, 2019 at 10:49:57AM +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/[email protected]
> >
> > Queued, thank you!
> >
> > Adding the checkpatch maintainers on CC as well. The "do { } while
> > (0)" prevents the return value from being used, by design. Given the
> > checkpatch complaint, is there some better way to achieve this?
>
> Not sure what the checkpatch complaint is here.
Checkpatch seems to want at least two statements in each
"do { } while (0)" macro definition:
WARNING: Single statement macros should not use a do {} while (0) loop
> Reading the link above, there seems to be a compiler warning.
The compiler warning is a theoretical issue that is being fixed by this
patch, and the patch is giving the checkpatch warning.
> Perhaps a statement expression macro with no return value?
>
> #define rcu_assign_pointer(p, v) ({ (p) = (v); ; })
This is at best an acquired taste for me...
Thanx, Paul
On Mon, 2019-05-27 at 10:49 -0700, Paul E. McKenney wrote:
> On Mon, May 27, 2019 at 10:21:22AM -0700, Joe Perches wrote:
> > On Mon, 2019-05-27 at 09:10 -0700, Paul E. McKenney wrote:
> > > On Mon, May 27, 2019 at 10:49:57AM +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/[email protected]
> > >
> > > Queued, thank you!
> > >
> > > Adding the checkpatch maintainers on CC as well. The "do { } while
> > > (0)" prevents the return value from being used, by design. Given the
> > > checkpatch complaint, is there some better way to achieve this?
> >
> > Not sure what the checkpatch complaint is here.
>
> Checkpatch seems to want at least two statements in each
> "do { } while (0)" macro definition:
>
> WARNING: Single statement macros should not use a do {} while (0) loop
>
> > Reading the link above, there seems to be a compiler warning.
>
> The compiler warning is a theoretical issue that is being fixed by this
> patch, and the patch is giving the checkpatch warning.
>
> > Perhaps a statement expression macro with no return value?
> >
> > #define rcu_assign_pointer(p, v) ({ (p) = (v); ; })
>
> This is at best an acquired taste for me...
Another ugly possibility could be:
#define rcu_assign_pointer(p, v) do {if (1) (p) = (v); } while (0)
Possibly the best option would be to ignore checkpatch here
and just add a comment above the use.
On Mon, May 27, 2019 at 10:57:52AM -0700, Joe Perches wrote:
> On Mon, 2019-05-27 at 10:49 -0700, Paul E. McKenney wrote:
> > On Mon, May 27, 2019 at 10:21:22AM -0700, Joe Perches wrote:
> > > On Mon, 2019-05-27 at 09:10 -0700, Paul E. McKenney wrote:
> > > > On Mon, May 27, 2019 at 10:49:57AM +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/[email protected]
> > > >
> > > > Queued, thank you!
> > > >
> > > > Adding the checkpatch maintainers on CC as well. The "do { } while
> > > > (0)" prevents the return value from being used, by design. Given the
> > > > checkpatch complaint, is there some better way to achieve this?
> > >
> > > Not sure what the checkpatch complaint is here.
> >
> > Checkpatch seems to want at least two statements in each
> > "do { } while (0)" macro definition:
> >
> > WARNING: Single statement macros should not use a do {} while (0) loop
> >
> > > Reading the link above, there seems to be a compiler warning.
> >
> > The compiler warning is a theoretical issue that is being fixed by this
> > patch, and the patch is giving the checkpatch warning.
> >
> > > Perhaps a statement expression macro with no return value?
> > >
> > > #define rcu_assign_pointer(p, v) ({ (p) = (v); ; })
> >
> > This is at best an acquired taste for me...
>
> Another ugly possibility could be:
>
> #define rcu_assign_pointer(p, v) do {if (1) (p) = (v); } while (0)
And, not to be left out, another ugly possibility might be:
#define rcu_assign_pointer(p, v) ((void)((p) = (v)))
> Possibly the best option would be to ignore checkpatch here
> and just add a comment above the use.
Works for me!
Thanx, Paul