Re: [PATCH v2] rcu: Provide a boot time parameter to control lazy RCU

[Uladzislau Rezki]

On Tue, Dec 12, 2023 at 05:28:54PM -0500, Joel Fernandes wrote:
> On Tue, Dec 12, 2023 at 7:35 AM Uladzislau Rezki <urezki@xxxxxxxxx> wrote:
> >
> > On Sun, Dec 03, 2023 at 01:12:52AM +0000, Qais Yousef wrote:
> > > To allow more flexible arrangements while still provide a single kernel
> > > for distros, provide a boot time parameter to enable/disable lazy RCU.
> > >
> > > Specify:
> > >
> > >       rcutree.enable_rcu_lazy=[y|1|n|0]
> > >
> > > Which also requires
> > >
> > >       rcu_nocbs=all
> > >
> > > at boot time to enable/disable lazy RCU.
> > >
> > > To disable it by default at build time when CONFIG_RCU_LAZY=y, the new
> > > CONFIG_RCU_LAZY_DEFAULT_OFF can be used.
> > >
> > > Signed-off-by: Qais Yousef (Google) <qyousef@xxxxxxxxxxx>
> > > ---
> > >
> > > Changes since v1:
> > >
> > >       * Use module_param() instead of module_param_cb()
> > >       * Add new CONFIG_RCU_LAZY_DEFAULT_OFF to force default off.
> > >       * Remove unnecessary READ_ONCE()
> > >
> > > Tested on qemu only this time with various config/boot configuration to ensure
> > > expected values are in sysfs.
> > >
> > > Did a bunch of build tests against various configs/archs.
> > >
> > >  Documentation/admin-guide/kernel-parameters.txt |  5 +++++
> > >  kernel/rcu/Kconfig                              | 13 +++++++++++++
> > >  kernel/rcu/tree.c                               |  7 ++++++-
> > >  3 files changed, 24 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/Kconfig b/kernel/rcu/Kconfig
> > > index bdd7eadb33d8..e7d2dd267593 100644
> > > --- a/kernel/rcu/Kconfig
> > > +++ b/kernel/rcu/Kconfig
> > > @@ -314,6 +314,19 @@ config RCU_LAZY
> > >         To save power, batch RCU callbacks and flush after delay, memory
> > >         pressure, or callback list growing too big.
> > >
> > > +       Requires rcu_nocbs=all to be set.
> > > +
> > > +       Use rcutree.enable_rcu_lazy=0 to turn it off at boot time.
> > > +
> > > +config RCU_LAZY_DEFAULT_OFF
> > > +     bool "Turn RCU lazy invocation off by default"
> > > +     depends on RCU_LAZY
> > > +     default n
> > > +     help
> > > +       Allows building the kernel with CONFIG_RCU_LAZY=y yet keep it default
> > > +       off. Boot time param rcutree.enable_rcu_lazy=1 can be used to switch
> > > +       it back on.
> > > +
> > >  config RCU_DOUBLE_CHECK_CB_TIME
> > >       bool "RCU callback-batch backup time check"
> > >       depends on RCU_EXPERT
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 3ac3c846105f..8b7675624815 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -2719,6 +2719,9 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> > >  }
> > >
> > >  #ifdef CONFIG_RCU_LAZY
> > > +static bool enable_rcu_lazy __read_mostly = !IS_ENABLED(CONFIG_RCU_LAZY_DEFAULT_OFF);
> > > +module_param(enable_rcu_lazy, bool, 0444);
> > > +
> > >  /**
> > >   * call_rcu_hurry() - Queue RCU callback for invocation after grace period, and
> > >   * flush all lazy callbacks (including the new one) to the main ->cblist while
> > > @@ -2744,6 +2747,8 @@ void call_rcu_hurry(struct rcu_head *head, rcu_callback_t func)
> > >       __call_rcu_common(head, func, false);
> > >  }
> > >  EXPORT_SYMBOL_GPL(call_rcu_hurry);
> > > +#else
> > > +#define enable_rcu_lazy              false
> > >  #endif
> > >
> > >  /**
> > > @@ -2792,7 +2797,7 @@ EXPORT_SYMBOL_GPL(call_rcu_hurry);
> > >   */
> > >  void call_rcu(struct rcu_head *head, rcu_callback_t func)
> > >  {
> > > -     __call_rcu_common(head, func, IS_ENABLED(CONFIG_RCU_LAZY));
> > > +     __call_rcu_common(head, func, enable_rcu_lazy);
> > >  }
> > >  EXPORT_SYMBOL_GPL(call_rcu);
> > >
> > I think, it makes sense. Especially for devices/systems where it is hard
> > to recompile the kernel and deploy it. For example, Google and GKI approach.
> 
> My concerns had nothing to do with recompiling the kernel. Passing a
> boot parameter (without a kernel compile) can just as well
> default-disable the feature.
> 
> I think what Qais is saying is that passing a boot parameter is itself
> a hassle in Android (something I did not know about) because of GKI
> etc.
>
That is true. Doing:

echo 1 > /sys/.../enable_lazy

is a way how to make it easy and flexible.

--
Uladzislau Rezki