[v1] SRCU-lite changes for v6.13

[PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

Posted by Paul E. McKenney 1 year, 4 months ago

Currently, there are only two flavors of readers, normal and NMI-safe.
A number of fields, functions, and types reflect this restriction.
This renaming-only commit prepares for the addition of light-weight
(as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
white-space fixeup!

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     | 21 ++++++++++-----------
 include/linux/srcutree.h |  2 +-
 kernel/rcu/srcutree.c    | 22 +++++++++++-----------
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 835bbb2d1f88a..06728ef6f32a4 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
 #define SRCU_NMI_SAFE		0x2
 
 #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
-void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
+void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
 #else
-static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
-					 bool nmi_safe) { }
+static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
 #endif
 
 
@@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	retval = __srcu_read_lock(ssp);
 	srcu_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, true);
+	srcu_check_read_flavor(ssp, true);
 	retval = __srcu_read_lock_nmisafe(ssp);
 	rcu_try_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	retval = __srcu_read_lock(ssp);
 	return retval;
 }
@@ -303,7 +302,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_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	return __srcu_read_lock(ssp);
 }
 
@@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	srcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock(ssp, idx);
 }
@@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_nmi_safety(ssp, true);
+	srcu_check_read_flavor(ssp, true);
 	rcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock_nmisafe(ssp, idx);
 }
@@ -343,7 +342,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_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	__srcu_read_unlock(ssp, idx);
 }
 
@@ -360,7 +359,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_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	__srcu_read_unlock(ssp, idx);
 }
 
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index ed57598394de3..ab7d8d215b84b 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -25,7 +25,7 @@ struct srcu_data {
 	/* Read-side state. */
 	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
 	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
-	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
+	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index e29c6cbffbcb0..18f2eae5e14bd 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 
 		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
 		if (IS_ENABLED(CONFIG_PROVE_RCU))
-			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
+			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
 	}
 	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
 		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
@@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
 
 #ifdef CONFIG_PROVE_RCU
 /*
- * Check for consistent NMI safety.
+ * Check for consistent reader flavor.
  */
-void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
+void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
 {
-	int nmi_safe_mask = 1 << nmi_safe;
-	int old_nmi_safe_mask;
+	int reader_flavor_mask = 1 << read_flavor;
+	int old_reader_flavor_mask;
 	struct srcu_data *sdp;
 
 	/* NMI-unsafe use in NMI is a bad sign */
-	WARN_ON_ONCE(!nmi_safe && in_nmi());
+	WARN_ON_ONCE(!read_flavor && in_nmi());
 	sdp = raw_cpu_ptr(ssp->sda);
-	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
-	if (!old_nmi_safe_mask) {
-		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
+	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
+	if (!old_reader_flavor_mask) {
+		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
 		return;
 	}
-	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
+	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);
 }
-EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
+EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
 #endif /* CONFIG_PROVE_RCU */
 
 /*
-- 
2.40.1

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

Posted by Neeraj Upadhyay 1 year, 3 months ago

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> Currently, there are only two flavors of readers, normal and NMI-safe.
> A number of fields, functions, and types reflect this restriction.
> This renaming-only commit prepares for the addition of light-weight
> (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
> white-space fixeup!
> 
> 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     | 21 ++++++++++-----------
>  include/linux/srcutree.h |  2 +-
>  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
>  3 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 835bbb2d1f88a..06728ef6f32a4 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>  #define SRCU_NMI_SAFE		0x2
>  
>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>  #else
> -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
> -					 bool nmi_safe) { }
> +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
>  #endif
>  
>  
> @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);

As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
be better here?


>  	retval = __srcu_read_lock(ssp);
>  	srcu_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, true);
> +	srcu_check_read_flavor(ssp, true);
>  	retval = __srcu_read_lock_nmisafe(ssp);
>  	rcu_try_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	retval = __srcu_read_lock(ssp);
>  	return retval;
>  }
> @@ -303,7 +302,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_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	return __srcu_read_lock(ssp);
>  }
>  
> @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	srcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock(ssp, idx);
>  }
> @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_nmi_safety(ssp, true);
> +	srcu_check_read_flavor(ssp, true);
>  	rcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock_nmisafe(ssp, idx);
>  }
> @@ -343,7 +342,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_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> @@ -360,7 +359,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_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index ed57598394de3..ab7d8d215b84b 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -25,7 +25,7 @@ struct srcu_data {
>  	/* Read-side state. */
>  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
>  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
> -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
> +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */

This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?


- Neeraj

>  
>  	/* Update-side state. */
>  	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index e29c6cbffbcb0..18f2eae5e14bd 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>  
>  		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> -			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
> +			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>  	}
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
>  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> @@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>  
>  #ifdef CONFIG_PROVE_RCU
>  /*
> - * Check for consistent NMI safety.
> + * Check for consistent reader flavor.
>   */
> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>  {
> -	int nmi_safe_mask = 1 << nmi_safe;
> -	int old_nmi_safe_mask;
> +	int reader_flavor_mask = 1 << read_flavor;
> +	int old_reader_flavor_mask;
>  	struct srcu_data *sdp;
>  
>  	/* NMI-unsafe use in NMI is a bad sign */
> -	WARN_ON_ONCE(!nmi_safe && in_nmi());
> +	WARN_ON_ONCE(!read_flavor && in_nmi());
>  	sdp = raw_cpu_ptr(ssp->sda);
> -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
> -	if (!old_nmi_safe_mask) {
> -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
> +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> +	if (!old_reader_flavor_mask) {
> +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>  		return;
>  	}
> -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
> +	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);
>  }
> -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
> +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>  #endif /* CONFIG_PROVE_RCU */
>  
>  /*

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

Posted by Paul E. McKenney 1 year, 3 months ago

On Mon, Oct 14, 2024 at 02:40:35PM +0530, Neeraj Upadhyay wrote:
> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > Currently, there are only two flavors of readers, normal and NMI-safe.
> > A number of fields, functions, and types reflect this restriction.
> > This renaming-only commit prepares for the addition of light-weight
> > (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
> > white-space fixeup!
> > 
> > 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     | 21 ++++++++++-----------
> >  include/linux/srcutree.h |  2 +-
> >  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
> >  3 files changed, 22 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 835bbb2d1f88a..06728ef6f32a4 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> >  #define SRCU_NMI_SAFE		0x2
> >  
> >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> > -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
> > +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> >  #else
> > -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
> > -					 bool nmi_safe) { }
> > +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
> >  #endif
> >  
> >  
> > @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> 
> As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
> be better here?

Agreed, and a later commit does introduce macros.

> >  	retval = __srcu_read_lock(ssp);
> >  	srcu_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, true);
> > +	srcu_check_read_flavor(ssp, true);
> >  	retval = __srcu_read_lock_nmisafe(ssp);
> >  	rcu_try_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	retval = __srcu_read_lock(ssp);
> >  	return retval;
> >  }
> > @@ -303,7 +302,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_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	return __srcu_read_lock(ssp);
> >  }
> >  
> > @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	srcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> > @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_nmi_safety(ssp, true);
> > +	srcu_check_read_flavor(ssp, true);
> >  	rcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock_nmisafe(ssp, idx);
> >  }
> > @@ -343,7 +342,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_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > @@ -360,7 +359,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_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index ed57598394de3..ab7d8d215b84b 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -25,7 +25,7 @@ struct srcu_data {
> >  	/* Read-side state. */
> >  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
> >  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
> > -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
> > +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
> 
> This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?

Yes, it is a mask, but one that should only ever have a single bit set.
So calling it a mask might or might not be a service to the reader.

							Thanx, Paul

> - Neeraj
> 
> >  
> >  	/* Update-side state. */
> >  	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index e29c6cbffbcb0..18f2eae5e14bd 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  
> >  		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > -			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
> > +			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> >  	}
> >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
> >  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > @@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> >  
> >  #ifdef CONFIG_PROVE_RCU
> >  /*
> > - * Check for consistent NMI safety.
> > + * Check for consistent reader flavor.
> >   */
> > -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
> > +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> >  {
> > -	int nmi_safe_mask = 1 << nmi_safe;
> > -	int old_nmi_safe_mask;
> > +	int reader_flavor_mask = 1 << read_flavor;
> > +	int old_reader_flavor_mask;
> >  	struct srcu_data *sdp;
> >  
> >  	/* NMI-unsafe use in NMI is a bad sign */
> > -	WARN_ON_ONCE(!nmi_safe && in_nmi());
> > +	WARN_ON_ONCE(!read_flavor && in_nmi());
> >  	sdp = raw_cpu_ptr(ssp->sda);
> > -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
> > -	if (!old_nmi_safe_mask) {
> > -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
> > +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > +	if (!old_reader_flavor_mask) {
> > +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
> >  		return;
> >  	}
> > -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
> > +	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);
> >  }
> > -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
> > +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> >  #endif /* CONFIG_PROVE_RCU */
> >  
> >  /*
>

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

Posted by Neeraj Upadhyay 1 year, 3 months ago


On 10/14/2024 10:22 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:40:35PM +0530, Neeraj Upadhyay wrote:
>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>> Currently, there are only two flavors of readers, normal and NMI-safe.
>>> A number of fields, functions, and types reflect this restriction.
>>> This renaming-only commit prepares for the addition of light-weight
>>> (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
>>> white-space fixeup!
>>>
>>> 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     | 21 ++++++++++-----------
>>>  include/linux/srcutree.h |  2 +-
>>>  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
>>>  3 files changed, 22 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>> index 835bbb2d1f88a..06728ef6f32a4 100644
>>> --- a/include/linux/srcu.h
>>> +++ b/include/linux/srcu.h
>>> @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>>>  #define SRCU_NMI_SAFE		0x2
>>>  
>>>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>>>  #else
>>> -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
>>> -					 bool nmi_safe) { }
>>> +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
>>>  #endif
>>>  
>>>  
>>> @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>
>> As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
>> be better here?
> 
> Agreed, and a later commit does introduce macros.
> 
>>>  	retval = __srcu_read_lock(ssp);
>>>  	srcu_lock_acquire(&ssp->dep_map);
>>>  	return retval;
>>> @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, true);
>>> +	srcu_check_read_flavor(ssp, true);
>>>  	retval = __srcu_read_lock_nmisafe(ssp);
>>>  	rcu_try_lock_acquire(&ssp->dep_map);
>>>  	return retval;
>>> @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	retval = __srcu_read_lock(ssp);
>>>  	return retval;
>>>  }
>>> @@ -303,7 +302,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_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	return __srcu_read_lock(ssp);
>>>  }
>>>  
>>> @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>>>  	__releases(ssp)
>>>  {
>>>  	WARN_ON_ONCE(idx & ~0x1);
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	srcu_lock_release(&ssp->dep_map);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>> @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>  	__releases(ssp)
>>>  {
>>>  	WARN_ON_ONCE(idx & ~0x1);
>>> -	srcu_check_nmi_safety(ssp, true);
>>> +	srcu_check_read_flavor(ssp, true);
>>>  	rcu_lock_release(&ssp->dep_map);
>>>  	__srcu_read_unlock_nmisafe(ssp, idx);
>>>  }
>>> @@ -343,7 +342,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_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>>  
>>> @@ -360,7 +359,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_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>>  
>>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>>> index ed57598394de3..ab7d8d215b84b 100644
>>> --- a/include/linux/srcutree.h
>>> +++ b/include/linux/srcutree.h
>>> @@ -25,7 +25,7 @@ struct srcu_data {
>>>  	/* Read-side state. */
>>>  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
>>>  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
>>> -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
>>> +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
>>
>> This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?
> 
> Yes, it is a mask, but one that should only ever have a single bit set.
> So calling it a mask might or might not be a service to the reader.
> 

Ok. The usage of reader_flavor as a shifted val here and without
shift as arg of srcu_check_read_flavor() was a bit confusing.


- Neeraj

...

>>>  
>>>  #ifdef CONFIG_PROVE_RCU
>>>  /*
>>> - * Check for consistent NMI safety.
>>> + * Check for consistent reader flavor.
>>>   */
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>>  {
>>> -	int nmi_safe_mask = 1 << nmi_safe;
>>> -	int old_nmi_safe_mask;
>>> +	int reader_flavor_mask = 1 << read_flavor;
>>> +	int old_reader_flavor_mask;
>>>  	struct srcu_data *sdp;
>>>  
>>>  	/* NMI-unsafe use in NMI is a bad sign */
>>> -	WARN_ON_ONCE(!nmi_safe && in_nmi());
>>> +	WARN_ON_ONCE(!read_flavor && in_nmi());
>>>  	sdp = raw_cpu_ptr(ssp->sda);
>>> -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
>>> -	if (!old_nmi_safe_mask) {
>>> -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
>>> +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>> +	if (!old_reader_flavor_mask) {
>>> +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>>>  		return;
>>>  	}
>>> -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
>>> +	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);
>>>  }
>>> -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
>>> +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>>>  #endif /* CONFIG_PROVE_RCU */
>>>  
>>>  /*
>>