On Fri, Sep 30, 2022 at 05:08:18PM +0206, John Ogness wrote:
> Hi Paul,
>
> On 2022-09-29, "Paul E. McKenney" <paulmck@xxxxxxxxxx> 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
