[v1] SRCU-lite changes for v6.13

[PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

Posted by Paul E. McKenney 1 month, 2 weeks ago

This commit changes a few "cpuc" variables to "sdp" to align wiht usage
elsewhere.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 include/linux/srcu.h     | 35 ++++++++++++++++++----------------
 include/linux/srcutree.h |  4 ++++
 kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
 3 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 06728ef6f32a4..84daaa33ea0ab 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
 
 #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-#define SRCU_NMI_UNKNOWN	0x0
-#define SRCU_NMI_UNSAFE		0x1
-#define SRCU_NMI_SAFE		0x2
-
 #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
 void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
 #else
@@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
  * a mutex that is held elsewhere while calling synchronize_srcu() or
  * synchronize_srcu_expedited().
  *
- * Note that srcu_read_lock() and the matching srcu_read_unlock() must
- * occur in the same context, for example, it is illegal to invoke
- * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
- * was invoked in process context.
+ * The return value from srcu_read_lock() must be passed unaltered
+ * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
+ * the matching srcu_read_unlock() must occur in the same context, for
+ * example, it is illegal to invoke srcu_read_unlock() in an irq handler
+ * if the matching srcu_read_lock() was invoked in process context.  Or,
+ * for that matter to invoke srcu_read_unlock() from one task and the
+ * matching srcu_read_lock() from another.
  */
 static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	retval = __srcu_read_lock(ssp);
 	srcu_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
  *
  * Enter an SRCU read-side critical section, but in an NMI-safe manner.
  * See srcu_read_lock() for more information.
+ *
+ * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
+ * then none of the other flavors may be used, whether before, during,
+ * or after.
  */
 static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, true);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
 	retval = __srcu_read_lock_nmisafe(ssp);
 	rcu_try_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	retval = __srcu_read_lock(ssp);
 	return retval;
 }
@@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
 {
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	return __srcu_read_lock(ssp);
 }
 
@@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	srcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock(ssp, idx);
 }
@@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_read_flavor(ssp, true);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
 	rcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock_nmisafe(ssp, idx);
 }
@@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 static inline notrace void
 srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
 {
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	__srcu_read_unlock(ssp, idx);
 }
 
@@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
 {
 	WARN_ON_ONCE(idx & ~0x1);
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	__srcu_read_unlock(ssp, idx);
 }
 
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index ab7d8d215b84b..79ad809c7f035 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -43,6 +43,10 @@ struct srcu_data {
 	struct srcu_struct *ssp;
 };
 
+/* Values for ->srcu_reader_flavor. */
+#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
+#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
+
 /*
  * Node in SRCU combining tree, similar in function to rcu_data.
  */
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index abe55777c4335..4c51be484b48a 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
 	}
 	return sum;
 }
@@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
+		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
 		if (IS_ENABLED(CONFIG_PROVE_RCU))
-			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
+			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
 	}
 	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
-		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
+		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
 	return sum;
 }
 
@@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
-		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
-		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
-		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
+		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
+		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
 	}
 	return sum;
 }
@@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
  */
 void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
 {
-	int reader_flavor_mask = 1 << read_flavor;
-	int old_reader_flavor_mask;
+	int old_read_flavor;
 	struct srcu_data *sdp;
 
-	/* NMI-unsafe use in NMI is a bad sign */
-	WARN_ON_ONCE(!read_flavor && in_nmi());
+	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
+	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
+	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
+
 	sdp = raw_cpu_ptr(ssp->sda);
-	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
-	if (!old_reader_flavor_mask) {
-		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
-		if (!old_reader_flavor_mask)
+	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
+	if (!old_read_flavor) {
+		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
+		if (!old_read_flavor)
 			return;
 	}
-	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
+	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
 }
 EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
 #endif /* CONFIG_PROVE_RCU */
-- 
2.40.1

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

Posted by Neeraj Upadhyay 1 month, 2 weeks ago

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> elsewhere.
> 

s/wiht/with/

This commit is doing a lot more than renaming "cpuc".



- Neeraj

> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---
>  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
>  include/linux/srcutree.h |  4 ++++
>  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
>  3 files changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 06728ef6f32a4..84daaa33ea0ab 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>  
>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
> -#define SRCU_NMI_UNKNOWN	0x0
> -#define SRCU_NMI_UNSAFE		0x1
> -#define SRCU_NMI_SAFE		0x2
> -
>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>  #else
> @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
>   * a mutex that is held elsewhere while calling synchronize_srcu() or
>   * synchronize_srcu_expedited().
>   *
> - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> - * occur in the same context, for example, it is illegal to invoke
> - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> - * was invoked in process context.
> + * The return value from srcu_read_lock() must be passed unaltered
> + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> + * the matching srcu_read_unlock() must occur in the same context, for
> + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> + * if the matching srcu_read_lock() was invoked in process context.  Or,
> + * for that matter to invoke srcu_read_unlock() from one task and the
> + * matching srcu_read_lock() from another.
>   */
>  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	retval = __srcu_read_lock(ssp);
>  	srcu_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>   *
>   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
>   * See srcu_read_lock() for more information.
> + *
> + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> + * then none of the other flavors may be used, whether before, during,
> + * or after.
>   */
>  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, true);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>  	retval = __srcu_read_lock_nmisafe(ssp);
>  	rcu_try_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	retval = __srcu_read_lock(ssp);
>  	return retval;
>  }
> @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	return __srcu_read_lock(ssp);
>  }
>  
> @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	srcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock(ssp, idx);
>  }
> @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_read_flavor(ssp, true);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>  	rcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock_nmisafe(ssp, idx);
>  }
> @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  static inline notrace void
>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>  {
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index ab7d8d215b84b..79ad809c7f035 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -43,6 +43,10 @@ struct srcu_data {
>  	struct srcu_struct *ssp;
>  };
>  
> +/* Values for ->srcu_reader_flavor. */
> +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> +
>  /*
>   * Node in SRCU combining tree, similar in function to rcu_data.
>   */
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index abe55777c4335..4c51be484b48a 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
>  	}
>  	return sum;
>  }
> @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>  	}
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>  	return sum;
>  }
>  
> @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
>  	}
>  	return sum;
>  }
> @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>   */
>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>  {
> -	int reader_flavor_mask = 1 << read_flavor;
> -	int old_reader_flavor_mask;
> +	int old_read_flavor;
>  	struct srcu_data *sdp;
>  
> -	/* NMI-unsafe use in NMI is a bad sign */
> -	WARN_ON_ONCE(!read_flavor && in_nmi());
> +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> +
>  	sdp = raw_cpu_ptr(ssp->sda);
> -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> -	if (!old_reader_flavor_mask) {
> -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> -		if (!old_reader_flavor_mask)
> +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> +	if (!old_read_flavor) {
> +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> +		if (!old_read_flavor)
>  			return;
>  	}
> -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
>  }
>  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>  #endif /* CONFIG_PROVE_RCU */

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

Posted by Paul E. McKenney 1 month, 1 week ago

On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> > elsewhere.
> > 
> 
> s/wiht/with/

Good eyes!

> This commit is doing a lot more than renaming "cpuc".

Indeed, it does look like I forgot to commit between two changes.

It looks like this commit log goes with the changes to the
functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
srcu_readers_active().  With the exception of the change from "NMI-safe"
to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().

How would you suggest that I split up the non-s/cpuc/sdp/ changes?

							Thanx, Paul

> - Neeraj
> 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: <bpf@vger.kernel.org>
> > ---
> >  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
> >  include/linux/srcutree.h |  4 ++++
> >  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
> >  3 files changed, 44 insertions(+), 36 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 06728ef6f32a4..84daaa33ea0ab 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> >  
> >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >  
> > -#define SRCU_NMI_UNKNOWN	0x0
> > -#define SRCU_NMI_UNSAFE		0x1
> > -#define SRCU_NMI_SAFE		0x2
> > -
> >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> >  #else
> > @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
> >   * a mutex that is held elsewhere while calling synchronize_srcu() or
> >   * synchronize_srcu_expedited().
> >   *
> > - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> > - * occur in the same context, for example, it is illegal to invoke
> > - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> > - * was invoked in process context.
> > + * The return value from srcu_read_lock() must be passed unaltered
> > + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> > + * the matching srcu_read_unlock() must occur in the same context, for
> > + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> > + * if the matching srcu_read_lock() was invoked in process context.  Or,
> > + * for that matter to invoke srcu_read_unlock() from one task and the
> > + * matching srcu_read_lock() from another.
> >   */
> >  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	retval = __srcu_read_lock(ssp);
> >  	srcu_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >   *
> >   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
> >   * See srcu_read_lock() for more information.
> > + *
> > + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> > + * then none of the other flavors may be used, whether before, during,
> > + * or after.
> >   */
> >  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, true);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> >  	retval = __srcu_read_lock_nmisafe(ssp);
> >  	rcu_try_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	retval = __srcu_read_lock(ssp);
> >  	return retval;
> >  }
> > @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	return __srcu_read_lock(ssp);
> >  }
> >  
> > @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	srcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> > @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_read_flavor(ssp, true);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> >  	rcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock_nmisafe(ssp, idx);
> >  }
> > @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  static inline notrace void
> >  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
> >  {
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index ab7d8d215b84b..79ad809c7f035 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -43,6 +43,10 @@ struct srcu_data {
> >  	struct srcu_struct *ssp;
> >  };
> >  
> > +/* Values for ->srcu_reader_flavor. */
> > +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> > +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> > +
> >  /*
> >   * Node in SRCU combining tree, similar in function to rcu_data.
> >   */
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index abe55777c4335..4c51be484b48a 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> >  	}
> >  	return sum;
> >  }
> > @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> > +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
> >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> > +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> >  	}
> >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> >  	return sum;
> >  }
> >  
> > @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
> >  	}
> >  	return sum;
> >  }
> > @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> >   */
> >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> >  {
> > -	int reader_flavor_mask = 1 << read_flavor;
> > -	int old_reader_flavor_mask;
> > +	int old_read_flavor;
> >  	struct srcu_data *sdp;
> >  
> > -	/* NMI-unsafe use in NMI is a bad sign */
> > -	WARN_ON_ONCE(!read_flavor && in_nmi());
> > +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> > +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> > +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> > +
> >  	sdp = raw_cpu_ptr(ssp->sda);
> > -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > -	if (!old_reader_flavor_mask) {
> > -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> > -		if (!old_reader_flavor_mask)
> > +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> > +	if (!old_read_flavor) {
> > +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> > +		if (!old_read_flavor)
> >  			return;
> >  	}
> > -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> > +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
> >  }
> >  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> >  #endif /* CONFIG_PROVE_RCU */
>

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

Posted by Paul E. McKenney 1 month, 1 week ago

On Mon, Oct 14, 2024 at 09:49:52AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
> > On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > > This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> > > elsewhere.
> > > 
> > 
> > s/wiht/with/
> 
> Good eyes!

And fixed.

> > This commit is doing a lot more than renaming "cpuc".
> 
> Indeed, it does look like I forgot to commit between two changes.
> 
> It looks like this commit log goes with the changes to the
> functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
> srcu_readers_active().  With the exception of the change from "NMI-safe"
> to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().
> 
> How would you suggest that I split up the non-s/cpuc/sdp/ changes?

As a first step, I split it three ways, as you can see on the updated "dev"
branch in the -rcu tree.

Thoughts?

							Thanx, Paul

> > - Neeraj
> > 
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > > Cc: <bpf@vger.kernel.org>
> > > ---
> > >  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
> > >  include/linux/srcutree.h |  4 ++++
> > >  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
> > >  3 files changed, 44 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > index 06728ef6f32a4..84daaa33ea0ab 100644
> > > --- a/include/linux/srcu.h
> > > +++ b/include/linux/srcu.h
> > > @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> > >  
> > >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >  
> > > -#define SRCU_NMI_UNKNOWN	0x0
> > > -#define SRCU_NMI_UNSAFE		0x1
> > > -#define SRCU_NMI_SAFE		0x2
> > > -
> > >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> > >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> > >  #else
> > > @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
> > >   * a mutex that is held elsewhere while calling synchronize_srcu() or
> > >   * synchronize_srcu_expedited().
> > >   *
> > > - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> > > - * occur in the same context, for example, it is illegal to invoke
> > > - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> > > - * was invoked in process context.
> > > + * The return value from srcu_read_lock() must be passed unaltered
> > > + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> > > + * the matching srcu_read_unlock() must occur in the same context, for
> > > + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> > > + * if the matching srcu_read_lock() was invoked in process context.  Or,
> > > + * for that matter to invoke srcu_read_unlock() from one task and the
> > > + * matching srcu_read_lock() from another.
> > >   */
> > >  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	retval = __srcu_read_lock(ssp);
> > >  	srcu_lock_acquire(&ssp->dep_map);
> > >  	return retval;
> > > @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> > >   *
> > >   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
> > >   * See srcu_read_lock() for more information.
> > > + *
> > > + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> > > + * then none of the other flavors may be used, whether before, during,
> > > + * or after.
> > >   */
> > >  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, true);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> > >  	retval = __srcu_read_lock_nmisafe(ssp);
> > >  	rcu_try_lock_acquire(&ssp->dep_map);
> > >  	return retval;
> > > @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	retval = __srcu_read_lock(ssp);
> > >  	return retval;
> > >  }
> > > @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> > >  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	WARN_ON_ONCE(in_nmi());
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	return __srcu_read_lock(ssp);
> > >  }
> > >  
> > > @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> > >  	__releases(ssp)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	srcu_lock_release(&ssp->dep_map);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > > @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> > >  	__releases(ssp)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > > -	srcu_check_read_flavor(ssp, true);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> > >  	rcu_lock_release(&ssp->dep_map);
> > >  	__srcu_read_unlock_nmisafe(ssp, idx);
> > >  }
> > > @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> > >  static inline notrace void
> > >  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
> > >  {
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > >  
> > > @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > >  	WARN_ON_ONCE(in_nmi());
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > >  
> > > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > > index ab7d8d215b84b..79ad809c7f035 100644
> > > --- a/include/linux/srcutree.h
> > > +++ b/include/linux/srcutree.h
> > > @@ -43,6 +43,10 @@ struct srcu_data {
> > >  	struct srcu_struct *ssp;
> > >  };
> > >  
> > > +/* Values for ->srcu_reader_flavor. */
> > > +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> > > +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> > > +
> > >  /*
> > >   * Node in SRCU combining tree, similar in function to rcu_data.
> > >   */
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index abe55777c4335..4c51be484b48a 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> > >  	}
> > >  	return sum;
> > >  }
> > > @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> > > +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
> > >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > > -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> > > +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> > >  	}
> > >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > > -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > > +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> > >  	return sum;
> > >  }
> > >  
> > > @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> > > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> > > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> > > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> > > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
> > >  	}
> > >  	return sum;
> > >  }
> > > @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> > >   */
> > >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> > >  {
> > > -	int reader_flavor_mask = 1 << read_flavor;
> > > -	int old_reader_flavor_mask;
> > > +	int old_read_flavor;
> > >  	struct srcu_data *sdp;
> > >  
> > > -	/* NMI-unsafe use in NMI is a bad sign */
> > > -	WARN_ON_ONCE(!read_flavor && in_nmi());
> > > +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> > > +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> > > +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> > > +
> > >  	sdp = raw_cpu_ptr(ssp->sda);
> > > -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > > -	if (!old_reader_flavor_mask) {
> > > -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> > > -		if (!old_reader_flavor_mask)
> > > +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> > > +	if (!old_read_flavor) {
> > > +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> > > +		if (!old_read_flavor)
> > >  			return;
> > >  	}
> > > -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> > > +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
> > >  }
> > >  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> > >  #endif /* CONFIG_PROVE_RCU */
> >

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

Posted by Neeraj Upadhyay 1 month, 1 week ago


On 10/15/2024 5:59 AM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 09:49:52AM -0700, Paul E. McKenney wrote:
>> On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
>>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>>> This commit changes a few "cpuc" variables to "sdp" to align wiht usage
>>>> elsewhere.
>>>>
>>>
>>> s/wiht/with/
>>
>> Good eyes!
> 
> And fixed.
> >>> This commit is doing a lot more than renaming "cpuc".
>>
>> Indeed, it does look like I forgot to commit between two changes.
>>
>> It looks like this commit log goes with the changes to the
>> functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
>> srcu_readers_active().  With the exception of the change from "NMI-safe"
>> to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().
>>
>> How would you suggest that I split up the non-s/cpuc/sdp/ changes?
> 
> As a first step, I split it three ways, as you can see on the updated "dev"
> branch in the -rcu tree.
> 
> Thoughts?
> 

Split looks good to me!


- Neeraj

> 							Thanx, Paul
> 
>>> - Neeraj
>>>
>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Kent Overstreet <kent.overstreet@linux.dev>
>>>> Cc: <bpf@vger.kernel.org>
>>>> ---
>>>>  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
>>>>  include/linux/srcutree.h |  4 ++++
>>>>  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
>>>>  3 files changed, 44 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>>> index 06728ef6f32a4..84daaa33ea0ab 100644
>>>> --- a/include/linux/srcu.h
>>>> +++ b/include/linux/srcu.h
>>>> @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>>>>  
>>>>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>>>>  
>>>> -#define SRCU_NMI_UNKNOWN	0x0
>>>> -#define SRCU_NMI_UNSAFE		0x1
>>>> -#define SRCU_NMI_SAFE		0x2
>>>> -
>>>>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>>>>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>>>>  #else
>>>> @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
>>>>   * a mutex that is held elsewhere while calling synchronize_srcu() or
>>>>   * synchronize_srcu_expedited().
>>>>   *
>>>> - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
>>>> - * occur in the same context, for example, it is illegal to invoke
>>>> - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
>>>> - * was invoked in process context.
>>>> + * The return value from srcu_read_lock() must be passed unaltered
>>>> + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
>>>> + * the matching srcu_read_unlock() must occur in the same context, for
>>>> + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
>>>> + * if the matching srcu_read_lock() was invoked in process context.  Or,
>>>> + * for that matter to invoke srcu_read_unlock() from one task and the
>>>> + * matching srcu_read_lock() from another.
>>>>   */
>>>>  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	retval = __srcu_read_lock(ssp);
>>>>  	srcu_lock_acquire(&ssp->dep_map);
>>>>  	return retval;
>>>> @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>>   *
>>>>   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
>>>>   * See srcu_read_lock() for more information.
>>>> + *
>>>> + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
>>>> + * then none of the other flavors may be used, whether before, during,
>>>> + * or after.
>>>>   */
>>>>  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, true);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>>>>  	retval = __srcu_read_lock_nmisafe(ssp);
>>>>  	rcu_try_lock_acquire(&ssp->dep_map);
>>>>  	return retval;
>>>> @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	retval = __srcu_read_lock(ssp);
>>>>  	return retval;
>>>>  }
>>>> @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(in_nmi());
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	return __srcu_read_lock(ssp);
>>>>  }
>>>>  
>>>> @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>>>>  	__releases(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	srcu_lock_release(&ssp->dep_map);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>> @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>>  	__releases(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>> -	srcu_check_read_flavor(ssp, true);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>>>>  	rcu_lock_release(&ssp->dep_map);
>>>>  	__srcu_read_unlock_nmisafe(ssp, idx);
>>>>  }
>>>> @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>>  static inline notrace void
>>>>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>>>>  {
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>>  
>>>> @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>>  	WARN_ON_ONCE(in_nmi());
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>>  
>>>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>>>> index ab7d8d215b84b..79ad809c7f035 100644
>>>> --- a/include/linux/srcutree.h
>>>> +++ b/include/linux/srcutree.h
>>>> @@ -43,6 +43,10 @@ struct srcu_data {
>>>>  	struct srcu_struct *ssp;
>>>>  };
>>>>  
>>>> +/* Values for ->srcu_reader_flavor. */
>>>> +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
>>>> +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
>>>> +
>>>>  /*
>>>>   * Node in SRCU combining tree, similar in function to rcu_data.
>>>>   */
>>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>>>> index abe55777c4335..4c51be484b48a 100644
>>>> --- a/kernel/rcu/srcutree.c
>>>> +++ b/kernel/rcu/srcutree.c
>>>> @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
>>>>  	}
>>>>  	return sum;
>>>>  }
>>>> @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
>>>> +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
>>>>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
>>>> -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>>>> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>>>>  	}
>>>>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
>>>> -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
>>>> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>>>>  	return sum;
>>>>  }
>>>>  
>>>> @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
>>>> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
>>>> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
>>>> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
>>>> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
>>>>  	}
>>>>  	return sum;
>>>>  }
>>>> @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>>>>   */
>>>>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>>>  {
>>>> -	int reader_flavor_mask = 1 << read_flavor;
>>>> -	int old_reader_flavor_mask;
>>>> +	int old_read_flavor;
>>>>  	struct srcu_data *sdp;
>>>>  
>>>> -	/* NMI-unsafe use in NMI is a bad sign */
>>>> -	WARN_ON_ONCE(!read_flavor && in_nmi());
>>>> +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
>>>> +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
>>>> +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
>>>> +
>>>>  	sdp = raw_cpu_ptr(ssp->sda);
>>>> -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>>> -	if (!old_reader_flavor_mask) {
>>>> -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
>>>> -		if (!old_reader_flavor_mask)
>>>> +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
>>>> +	if (!old_read_flavor) {
>>>> +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
>>>> +		if (!old_read_flavor)
>>>>  			return;
>>>>  	}
>>>> -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>>>> +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>>>>  #endif /* CONFIG_PROVE_RCU */
>>>