Re: [PATCH rcu 2/3] srcu: Check for srcu_read_lock_lite() across all CPUs

[Paul E. McKenney]

On Tue, Nov 12, 2024 at 08:58:08AM +0530, Neeraj Upadhyay wrote:
> On 11/12/2024 7:01 AM, Paul E. McKenney wrote:
> > If srcu_read_lock_lite() is used on a given srcu_struct structure, then
> > the grace-period processing must to synchronize_rcu() instead of smp_mb()
> 
> s/to/do/

Good eyes, fixed!

> > between the scans of the ->srcu_unlock_count[] and ->srcu_lock_count[]
> > counters.  Currently, it does that by testing the SRCU_READ_FLAVOR_LITE
> > bit of the ->srcu_reader_flavor mask, which works well.  But only if
> > the CPU running that srcu_struct structure's grace period has previously
> > executed srcu_read_lock_lite(), which might not be the case, especially
> > just after that srcu_struct structure has been created and initialized.
> > 
> > This commit therefore updates the srcu_readers_unlock_idx() function
> > to OR together the ->srcu_reader_flavor masks from all CPUs, and
> > then make the srcu_readers_active_idx_check() function that test the
> > SRCU_READ_FLAVOR_LITE bit in the resulting mask.
> > 
> > Note that the srcu_readers_unlock_idx() function is already scanning all
> > the CPUs to sum up the ->srcu_unlock_count[] fields and that this is on
> > the grace-period slow path, hence no concerns about the small amount of
> > extra work.
> > 
> > Reported-by: Neeraj Upadhyay <Neeraj.Upadhyay@xxxxxxx>
> > Closes: https://lore.kernel.org/all/d07e8f4a-d5ff-4c8e-8e61-50db285c57e9@xxxxxxx/
> > Fixes: c0f08d6b5a61 ("srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()")
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> > ---
> >  kernel/rcu/srcutree.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 70979f294768c..5991381b44383 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -458,7 +458,7 @@ static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, uns
> >   * Returns approximate total of the readers' ->srcu_unlock_count[] values
> >   * for the rank of per-CPU counters specified by idx.
> >   */
> > -static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> > +static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx, unsigned long *rdm)
> >  {
> >  	int cpu;
> >  	unsigned long mask = 0;
> > @@ -468,11 +468,11 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> >  		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
> > -		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > -			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> > +		mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> >  	}
> >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> >  		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> > +	*rdm = mask;
> >  	return sum;
> >  }
> >  
> > @@ -482,10 +482,11 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >   */
> >  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >  {
> > -	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
> > +	unsigned long rdm;
> >  	unsigned long unlocks;
> >  
> > -	unlocks = srcu_readers_unlock_idx(ssp, idx);
> > +	unlocks = srcu_readers_unlock_idx(ssp, idx, &rdm);
> > +	bool did_gp = !!(rdm & SRCU_READ_FLAVOR_LITE);
> 
> Move "did_gp" declaration up?

C now allows this?  ;-)

Fixed!

> Reviewed-by: Neeraj Upadhyay <Neeraj.Upadhyay@xxxxxxx>

And applied all three, again, thank you!

							Thanx, Paul