Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 08:35:06 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@fb.com, rostedt@goodmis.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and
 ->srcu_unlock_count to atomic
Message-ID: <20220930153506.GD4196@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220929180714.GA2874192@paulmck-ThinkPad-P17-Gen-1>
 <20220929180731.2875722-1-paulmck@kernel.org>
 <87ill4vrb9.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87ill4vrb9.fsf@jogness.linutronix.de>
Precedence: bulk

On Fri, Sep 30, 2022 at 05:08:18PM +0206, John Ogness wrote:
> Hi Paul,
> 
> On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 1c304fec89c0..6fd0665f4d1f 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
> >  	int idx;
> >  
> >  	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> > -	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> > +	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
> >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> >  	return idx;
> >  }
> 
> Is there any particular reason that you are directly modifying @counter
> instead of raw_cpu_ptr()+atomic_long_inc() that do you in
> __srcu_read_lock_nmisafe() of patch 2?

Performance.  From what I can see, this_cpu_inc() is way faster than
atomic_long_inc() on x86 and s390.  Maybe also on loongarch.  No idea
on arm64.

> > @@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
> >  void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  {
> >  	smp_mb(); /* C */  /* Avoid leaking the critical section. */
> > -	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
> > +	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
> >  }
> >  EXPORT_SYMBOL_GPL(__srcu_read_unlock);
> 
> Ditto.

Ditto back at you!  ;-)

> > @@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
> >  			struct srcu_data *sdp;
> >  
> >  			sdp = per_cpu_ptr(ssp->sda, cpu);
> > -			u0 = data_race(sdp->srcu_unlock_count[!idx]);
> > -			u1 = data_race(sdp->srcu_unlock_count[idx]);
> > +			u0 = data_race(sdp->srcu_unlock_count[!idx].counter);
> > +			u1 = data_race(sdp->srcu_unlock_count[idx].counter);
> >  
> >  			/*
> >  			 * Make sure that a lock is always counted if the corresponding
> 
> And instead of atomic_long_read().

You are right, here I could just as well use atomic_long_read().

> > @@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
> >  			 */
> >  			smp_rmb();
> >  
> > -			l0 = data_race(sdp->srcu_lock_count[!idx]);
> > -			l1 = data_race(sdp->srcu_lock_count[idx]);
> > +			l0 = data_race(sdp->srcu_lock_count[!idx].counter);
> > +			l1 = data_race(sdp->srcu_lock_count[idx].counter);
> >  
> >  			c0 = l0 - u0;
> >  			c1 = l1 - u1;
> 
> Ditto.

And here as well.  ;-)

I will fix these, and thank you for looking this over!

							Thanx, Paul