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@xxxxxxxxxxx>
> > ---
> >
> > 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 :-)
>
> 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
Jokes aside, all options work for me. I'll wait to hear from the other rcu
gurus what they'd like.
>
> 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
> And then maybe also a __read_mostly on the definition of enable_rcu_lazy?
+1
I think the READ_ONCE() was unnecessary too.
Thanks!
--
Qais Yousef
