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, 7 Jul 2023 09:05:12 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Z qiang <qiang.zhang1211@gmail.com>
Cc:     frederic@kernel.org, quic_neeraju@quicinc.com,
        joel@joelfernandes.org, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] srcu: Make srcu_might_be_idle() take early return if
 rcu_gp_is_normal() return true
Message-ID: <087e3da2-ffd9-4cf4-8893-73f4edc3cfba@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20230704082615.7415-1-qiang.zhang1211@gmail.com>
 <77d16cbb-b28c-4b13-aecb-9ad787559557@paulmck-laptop>
 <CALm+0cW6DojcxSnzf2mMXhXP-MHJ0aEvSqyh1TbtvBC4bzfsOw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALm+0cW6DojcxSnzf2mMXhXP-MHJ0aEvSqyh1TbtvBC4bzfsOw@mail.gmail.com>
Precedence: bulk

On Fri, Jul 07, 2023 at 06:28:29PM +0800, Z qiang wrote:
> >
> > On Tue, Jul 04, 2023 at 04:26:15PM +0800, Zqiang wrote:
> > > When invoke synchronize_srcu(), in the srcu_might_be_idle(), the current
> > > CPU's sdp->lock will be acquired to check whether there are pending
> > > callbacks in the sdp->srcu_cblist, if there are no pending callbacks,
> > > probabilistically probe global state to decide whether to convert to
> > > synchronize_srcu_expedited() call. however, for the rcupdate.rcu_normal=1
> > > kernels and after the rcu_set_runtime_mode() is called, invoke the
> > > rcu_gp_is_normal() is always return true, this mean that invoke the
> > > synchronize_srcu_expedited() always fall back to synchronize_srcu(),
> > > so there is no need to acquire sdp->lock to check sdp->srcu_cblist and
> > > probe global state in srcu_might_be_idle().
> > >
> > > This commit therefore make srcu_might_be_idle() return immediately if the
> > > rcu_gp_is_normal() return true.
> > >
> > > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > > ---
> > >  kernel/rcu/srcutree.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index 20d7a238d675..aea49cb60a45 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -1172,6 +1172,8 @@ static bool srcu_might_be_idle(struct srcu_struct *ssp)
> > >       unsigned long tlast;
> > >
> > >       check_init_srcu_struct(ssp);
> > > +     if (rcu_gp_is_normal())
> > > +             return false;
> >
> > Again, thank you for looking into SRCU!
> >
> > I am not at all enthusiastic about this one.  With this change, the name
> > srcu_might_be_idle() is no longer accurate.  Yes, the name could change,
> > but any name would be longer and more confusing.
> >
> > So unless there is a measureable benefit to this one on a production
> > workload, I cannot justify taking it.
> >
> > Is there a measureable benefit?
> 
> Hi, Paul
> 
> I only find that for Preempt-RT kernel,  the rcu_normal_after_boot is
> set by default:
> static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT);
> This affects only rcu but also srcu, this make the synchronize_srcu() and
> synchronize_srcu_expedited() always fall back to __synchronize_srcu(ssp, true),
> this means that call the srcu_might_be_idle() is meaningless.

I do understand that the current setup favors default kernel builds at
runtime by a few low-cost instructions, and that your change favors,
as you say, kernels built for real-time, kernels built for certain types
of HPC workloads, and all kernels during a small time during boot.

My question is instead whether any of this makes a measureable difference
at the system level.

My guess is "no, not even close", but the way to convince me otherwise
would be to actually run the workload and kernels on real hardware and
provide measurements showing a statistically significant difference that
the workload(s) in question care(s) about.

So what can you show me?

And srcu_might_be_idle() is not meaningless in that situation, just
ignored completely.  And that is in fact the nature and purpose of the
C-language || operator.  ;-)

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >                                                         Thanx, Paul
> >
> > >       /* If the local srcu_data structure has callbacks, not idle.  */
> > >       sdp = raw_cpu_ptr(ssp->sda);
> > >       spin_lock_irqsave_rcu_node(sdp, flags);
> > > --
> > > 2.17.1
> > >