Message-ID: <8ad4e628-7a88-490c-a54d-df09f851f189@paulmck-laptop>
Date:   Wed, 22 Nov 2023 15:26:17 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Qais Yousef <qyousef@...alina.io>
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Josh Triplett <josh@...htriplett.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Zqiang <qiang.zhang1211@...il.com>,
        Andrea Righi <andrea.righi@...onical.com>,
        John Stultz <jstultz@...gle.com>, linux-kernel@...r.kernel.org,
        rcu@...r.kernel.org
Subject: Re: [PATCH] rcu: Provide a boot time parameter to enable lazy RCU

On Tue, Nov 21, 2023 at 09:44:15PM +0000, Qais Yousef wrote:
> On 11/22/23 14:00, Paul E. McKenney wrote:
> > On Tue, Nov 21, 2023 at 08:53:04PM +0000, Qais Yousef wrote:
> > > To allow more flexible opt-in arrangements while still provide a single
> > > kernel for distros, provide a boot time parameter to enable lazy RCU.
> > > 
> > > Specify:
> > > 
> > > 	rcutree.enable_rcu_lazy
> > > 
> > > Which also requires
> > > 
> > > 	rcu_nocbs=all
> > > 
> > > at boot time to enable lazy RCU assuming CONFIG_RCU_LAZY=y. The
> > > parameter will be ignored if CONFIG_RCU_LAZY is not set.
> > > 
> > > With this change now lazy RCU is disabled by default if the boot
> > > parameter is not set even when CONFIG_RCU_LAZY is enabled.
> > > 
> > > Signed-off-by: Qais Yousef (Google) <qyousef@...alina.io>
> > > ---
> > > 
> > > Makes sense to remove the CONFIG_RCU_LAZY now we have a boot time param?
> > > 
> > > We can make it a static key too if it *really* matters.
> > > 
> > > Thanks to Joel for helping initially in reviewing this patch which was intended
> > > originally for Android.
> > > 
> > > I got some requests to make this a runtime modifiable for init scripts; but
> > > Paul suggested there shall be dragons. So RO it is.
> > 
> > I must defer to the people using this, but my experience is that kernel
> > boot parameters work for some people but not others.  For example,
> > I tried making rcu_nocbs be the only way to say that all CPUs were
> > going to be offloaded, but popular demand resulted in my adding a
> > CONFIG_RCU_NOCB_CPU_DEFAULT_ALL.
> 
> Speak of pleasing a crowd.. There's always someone who wants something else :-)
> 
> I imagine the difficulty is in some environments it is easier to switch a sysfs
> knob than add a new boot time parameter. And in the absence of a writable sysfs
> node, I can imagine some folks think having a Kconfig to force a default at
> compile time is the 2nd best compared to modifying their boot time parameters..
> 
> Either way; I'll follow what the crowd wants too :-)

Usually a wise choice.  ;-)

But I must defer to the people using it.

> > If we cannot be sure that we know everyone using CONFIG_RCU_LAZY=y
> > and expecting full laziness, the safe approach is to make another
> > Kconfig option that defaults to off, but with either setting allowing
> > rcutree.enable_rcu_lazy to override at boot time.
> > 
> > If you can be sure that you know everyone using CONFIG_RCU_LAZY=y
> > is OK with this change, I must confess that I am curious as to how
> > you found them all.
> 
> If you let it break and no one shouts..
> 
> /me hides

Indeed, sometimes you get lucky.  Sometimes.  ;-)

> Jokes aside, all options work for me. I'll wait to hear from the other rcu
> gurus what they'd like.

Fair enough!

> > Thoughts?
> > 
> > 							Thanx, Paul
> > 
> > >  .../admin-guide/kernel-parameters.txt         |  5 ++++
> > >  kernel/rcu/tree.c                             | 26 ++++++++++++++++++-
> > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 65731b060e3f..2f0386a12aa7 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -5021,6 +5021,11 @@
> > >  			this kernel boot parameter, forcibly setting it
> > >  			to zero.
> > >  
> > > +	rcutree.enable_rcu_lazy= [KNL]
> > > +			To save power, batch RCU callbacks and flush after
> > > +			delay, memory pressure or callback list growing too
> > > +			big.
> > > +
> > >  	rcuscale.gp_async= [KNL]
> > >  			Measure performance of asynchronous
> > >  			grace-period primitives such as call_rcu().
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 3ac3c846105f..e0885905b3f6 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -2718,7 +2718,30 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> > >  	}
> > >  }
> > >  
> > > +static bool enable_rcu_lazy;
> > >  #ifdef CONFIG_RCU_LAZY
> > > +/* Enable lazy rcu at boot time */
> > > +static int param_set_rcu_lazy(const char *val, const struct kernel_param *kp)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Make sure a grace period has passed before and after flipping the
> > > +	 * switch.
> > > +	 */
> > > +	rcu_barrier();
> > > +	ret = param_set_bool(val, kp);
> > > +	rcu_barrier();
> > > +
> > > +	return ret;
> > > +}
> > > +static const struct kernel_param_ops rcu_lazy_ops = {
> > > +	.flags = KERNEL_PARAM_OPS_FL_NOARG,
> > > +	.set = param_set_rcu_lazy,
> > > +	.get = param_get_bool,
> > > +};
> > > +module_param_cb(enable_rcu_lazy, &rcu_lazy_ops, &enable_rcu_lazy, 0444);
> > 
> > OK, I will bite...
> > 
> > Given that this is to be set only at boot time, why not replace everything
> > from "#ifdef CONFIG_RCU_LAZY" to here with this?
> > 
> > module_param(enable_rcu_lazy, bool, 0444);
> 
> No need for the rcu_barrier() then? Only reason why we use the _cb flavour

The module_param() parameters are processed during early boot, before
the boot CPU has enabled interrupts.  In fact, before rcu_init()
is invoked, which is in turn long before the scheduler has started.
Calling rcu_barrier() that early is not so good for your kernel's
actuarial statistics.

I am guessing that the module_param_cb() processing happens somewhat
later in the kernel's lifetime.

> > And then maybe also a __read_mostly on the definition of enable_rcu_lazy?
> 
> +1
> 
> I think the READ_ONCE() was unnecessary too.

Good point, and yes, I did miss that one.

							Thanx, Paul

Powered by blists - more mailing lists