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

  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@layalina.io>
---

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(-)
  

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@layalina.io>

Thanks! Everything looks good to me and I also verified that
rcutree.enable_rcu_lazy is enforcing the proper behavior.

FWIW:

Tested-by: Andrea Righi <andrea.righi@canonical.com>

> ---
> 
> 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);
>  
> -- 
> 2.34.1
  

On Sun, Dec 03, 2023 at 02:18:19PM +0100, Andrea Righi 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@layalina.io>
> 
> Thanks! Everything looks good to me and I also verified that
> rcutree.enable_rcu_lazy is enforcing the proper behavior.
> 
> FWIW:
> 
> Tested-by: Andrea Righi <andrea.righi@canonical.com>

Queued for v6.9 and further testing and review, thank you!

							Thanx, Paul

> > ---
> > 
> > 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);
> >  
> > -- 
> > 2.34.1
  

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@layalina.io>

Thanks Qais, I have a comment below:

> ---
> 
> 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.
> +

I think a better approach is not do an anti-CONFIG option and instead do
a shorter parameter "rcutree.lazy=0". If CONFIG_RCU_LAZY is set, then we can
just default to keeping lazy on. I'd like to avoid proliferation of already
large number of RCU config options and more chances of errors.

I also want lazy to be ON for everybody configuring it into the kernel by
default (those who don't want it just set CONFIG_RCU_LAZY=n), this is what
tglx also suggested that's why we made changed of our initial prototypes of
call_rcu_lazy() and instead we made call_rcu() to put everyone on the lazy
train and not add more APIs (thus causing more confusion to kernel
developers). This was a bit painful, but it was worth it.

>  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);

And then this can just be                    = true;

thanks,

 - Joel


> +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);
>  
> -- 
> 2.34.1
>
  

On 12/05/23 16:20, Joel Fernandes wrote:

> I think a better approach is not do an anti-CONFIG option and instead do
> a shorter parameter "rcutree.lazy=0". If CONFIG_RCU_LAZY is set, then we can
> just default to keeping lazy on. I'd like to avoid proliferation of already
> large number of RCU config options and more chances of errors.

The issue is that we don't want to ship with default on :-)

> I also want lazy to be ON for everybody configuring it into the kernel by
> default (those who don't want it just set CONFIG_RCU_LAZY=n), this is what

This is still the default behavior.

And all or nothing approach is not practical. You're telling me if I can't ship
with default off, then I must disable it altogether. Adoption will become
harder IMHO.

> tglx also suggested that's why we made changed of our initial prototypes of
> call_rcu_lazy() and instead we made call_rcu() to put everyone on the lazy
> train and not add more APIs (thus causing more confusion to kernel
> developers). This was a bit painful, but it was worth it.

I think implementation details make sense, but orthogonal to the problem of
enabling CONFIG_RCU_LAZY=y but still ship with default off. It is a risky
change and we want to start staging with default off first. Not allowing this
in upstream means I'll either have to resort to keep it disabled, or carry out
of tree patch to get what I want. Both of which would be unfortunate.


Thanks!

--
Qais Yousef
  

On 12/7/23 12:20, Qais Yousef wrote:
> On 12/05/23 16:20, Joel Fernandes wrote:
> 
>> I think a better approach is not do an anti-CONFIG option and instead do
>> a shorter parameter "rcutree.lazy=0". If CONFIG_RCU_LAZY is set, then we can
>> just default to keeping lazy on. I'd like to avoid proliferation of already
>> large number of RCU config options and more chances of errors.
> 
> The issue is that we don't want to ship with default on :-)

Yes, so you can ship with rcutree.enable_lazy=0 which this patch adds, no? In
theory, you can accomplish this by simply CONFIG_RCU_LAZY=y and
rcutree.enable_lazy=0 or rcutree.lazy=0.

However, I see the inconvenience factor (you have to set a boot parameter
without making this a purely .config affair) so I am not terribly opposed with
this patch (I am also guilty of adding a CONFIG option to avoid having to set a
boot parameter (for unrelated feature), but in my defense I did not know a boot
parameter existed for the said feature). ;-)

>> I also want lazy to be ON for everybody configuring it into the kernel by
>> default (those who don't want it just set CONFIG_RCU_LAZY=n), this is what
> 
> This is still the default behavior.
> 
> And all or nothing approach is not practical. You're telling me if I can't ship
> with default off, then I must disable it altogether. Adoption will become
> harder IMHO.

No, that's not what I said. You misunderstood me (which is probably my fault at
not being more clear). It is not all or nothing. I am just saying you can
accomplish "default off" by just setting the boot parameter. With this patch,
you are not willing to do that out of convenience, which I can understand but
still we should at least have a discussion about that.

> 
>> tglx also suggested that's why we made changed of our initial prototypes of
>> call_rcu_lazy() and instead we made call_rcu() to put everyone on the lazy
>> train and not add more APIs (thus causing more confusion to kernel
>> developers). This was a bit painful, but it was worth it.
> 
> I think implementation details make sense, but orthogonal to the problem of
> enabling CONFIG_RCU_LAZY=y but still ship with default off. It is a risky
> change and we want to start staging with default off first.

Never had any issue with that. I very much want to see this safely rolled out to
Android. ;-)

> Not allowing this
> in upstream means I'll either have to resort to keep it disabled, or carry out
> of tree patch to get what I want. Both of which would be unfortunate.

There is already precedent for building things into the kernel but keeping them
default off, so I don't have an issue with the experimentation use case. I was
just discussing whether the additional CONFIG is really needed when you already
have added a boot param to keep it default-off. If you have an argument for why
that would be really helpful [1].

Also, nit: rcutree.enable_lazy is probably better than rcutree.enable_rcu_lazy.
The 'rcu' is redundant.

Other than that, the patch LGTM but if you could update the commit log with
details about [1], that would be great. And while at it, you could add my tag:

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel
  

On 12/09/23 01:26, Joel Fernandes wrote:
> On 12/7/23 12:20, Qais Yousef wrote:
> > On 12/05/23 16:20, Joel Fernandes wrote:
> > 
> >> I think a better approach is not do an anti-CONFIG option and instead do
> >> a shorter parameter "rcutree.lazy=0". If CONFIG_RCU_LAZY is set, then we can
> >> just default to keeping lazy on. I'd like to avoid proliferation of already
> >> large number of RCU config options and more chances of errors.
> > 
> > The issue is that we don't want to ship with default on :-)
> 
> Yes, so you can ship with rcutree.enable_lazy=0 which this patch adds, no? In
> theory, you can accomplish this by simply CONFIG_RCU_LAZY=y and
> rcutree.enable_lazy=0 or rcutree.lazy=0.
> 
> However, I see the inconvenience factor (you have to set a boot parameter
> without making this a purely .config affair) so I am not terribly opposed with
> this patch (I am also guilty of adding a CONFIG option to avoid having to set a
> boot parameter (for unrelated feature), but in my defense I did not know a boot
> parameter existed for the said feature). ;-)

It is more than inconvenience. The GKI doesn't ship with a specific userspace.
So we can't guarantee the boot parameter will be set and have to rely on no one
missing the memo to add this additional parameter.

And to speed up adoption and testing, I am backporting the feature to 5.10,
5.15 and 6.1. It is risky enough to get a backport, but to default on it could
  introduce more subtle surprises. But not doing so we could end up waiting for
2 years before enough people move to the latest LTS that contains the feature.

> 
> >> I also want lazy to be ON for everybody configuring it into the kernel by
> >> default (those who don't want it just set CONFIG_RCU_LAZY=n), this is what
> > 
> > This is still the default behavior.
> > 
> > And all or nothing approach is not practical. You're telling me if I can't ship
> > with default off, then I must disable it altogether. Adoption will become
> > harder IMHO.
> 
> No, that's not what I said. You misunderstood me (which is probably my fault at
> not being more clear). It is not all or nothing. I am just saying you can
> accomplish "default off" by just setting the boot parameter. With this patch,
> you are not willing to do that out of convenience, which I can understand but
> still we should at least have a discussion about that.

Okay, sorry if I misunderstood.

> 
> > 
> >> tglx also suggested that's why we made changed of our initial prototypes of
> >> call_rcu_lazy() and instead we made call_rcu() to put everyone on the lazy
> >> train and not add more APIs (thus causing more confusion to kernel
> >> developers). This was a bit painful, but it was worth it.
> > 
> > I think implementation details make sense, but orthogonal to the problem of
> > enabling CONFIG_RCU_LAZY=y but still ship with default off. It is a risky
> > change and we want to start staging with default off first.
> 
> Never had any issue with that. I very much want to see this safely rolled out to
> Android. ;-)
> 
> > Not allowing this
> > in upstream means I'll either have to resort to keep it disabled, or carry out
> > of tree patch to get what I want. Both of which would be unfortunate.
> 
> There is already precedent for building things into the kernel but keeping them
> default off, so I don't have an issue with the experimentation use case. I was
> just discussing whether the additional CONFIG is really needed when you already
> have added a boot param to keep it default-off. If you have an argument for why
> that would be really helpful [1].
> 
> Also, nit: rcutree.enable_lazy is probably better than rcutree.enable_rcu_lazy.
> The 'rcu' is redundant.

It matches the config option so feels natural to have them both named the same?

> 
> Other than that, the patch LGTM but if you could update the commit log with
> details about [1], that would be great. And while at it, you could add my tag:

You forgot to include [1]? Or I'm just blind today?

> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

--
Qais Yousef
  

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@layalina.io>
> ---
> 
> 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.

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki
  

On Tue, Dec 12, 2023 at 7:35 AM Uladzislau Rezki <urezki@gmail.com> 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@layalina.io>
> > ---
> >
> > 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.

thanks,

 - Joel
  

On Tue, Dec 12, 2023 at 7:05 AM Qais Yousef <qyousef@layalina.io> wrote:
>
> On 12/09/23 01:26, Joel Fernandes wrote:
> > On 12/7/23 12:20, Qais Yousef wrote:
> > > On 12/05/23 16:20, Joel Fernandes wrote:
> > >
> > >> I think a better approach is not do an anti-CONFIG option and instead do
> > >> a shorter parameter "rcutree.lazy=0". If CONFIG_RCU_LAZY is set, then we can
> > >> just default to keeping lazy on. I'd like to avoid proliferation of already
> > >> large number of RCU config options and more chances of errors.
> > >
> > > The issue is that we don't want to ship with default on :-)
> >
> > Yes, so you can ship with rcutree.enable_lazy=0 which this patch adds, no? In
> > theory, you can accomplish this by simply CONFIG_RCU_LAZY=y and
> > rcutree.enable_lazy=0 or rcutree.lazy=0.
> >
> > However, I see the inconvenience factor (you have to set a boot parameter
> > without making this a purely .config affair) so I am not terribly opposed with
> > this patch (I am also guilty of adding a CONFIG option to avoid having to set a
> > boot parameter (for unrelated feature), but in my defense I did not know a boot
> > parameter existed for the said feature). ;-)
>
> It is more than inconvenience. The GKI doesn't ship with a specific userspace.
> So we can't guarantee the boot parameter will be set and have to rely on no one
> missing the memo to add this additional parameter.

Yes, I see that now. Looks like Android also needs to be supplying a
"GKI boot parameter" requirement to their GKI supplied kernels ;-).
But I see the issue you are referring to now.

It would be good to add these details to your respin.

> > > Not allowing this
> > > in upstream means I'll either have to resort to keep it disabled, or carry out
> > > of tree patch to get what I want. Both of which would be unfortunate.
> >
> > There is already precedent for building things into the kernel but keeping them
> > default off, so I don't have an issue with the experimentation use case. I was
> > just discussing whether the additional CONFIG is really needed when you already
> > have added a boot param to keep it default-off. If you have an argument for why
> > that would be really helpful [1].
> >
> > Also, nit: rcutree.enable_lazy is probably better than rcutree.enable_rcu_lazy.
> > The 'rcu' is redundant.
>
> It matches the config option so feels natural to have them both named the same?

Ok, either is fine with me.

> > Other than that, the patch LGTM but if you could update the commit log with
> > details about [1], that would be great. And while at it, you could add my tag:
>
> You forgot to include [1]? Or I'm just blind today?

Heh, if you search "[1]" you see it above where I said "helpful". ;-).
But apologies if I caused confusion.

thanks,

 - Joel


>
> >
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Thanks!
>
> --
> Qais Yousef
  

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@gmail.com> 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@layalina.io>
> > > ---
> > >
> > > 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
  

On Wed, Dec 13, 2023 at 5:35 AM Uladzislau Rezki <urezki@gmail.com> wrote:
[....]
> > > > +       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.

Hey Vlad, are you suggesting that the boot parameter be made to
support runtime? We can keep that for later as it may get complicated.
Qais's boot parameter is designed only for boot time.

Qais, could you resend the patch with our tags and updated description? Thanks,

 - Joel
  

Hello, Joel!

> [....]
> > > > > +       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.
> 
> Hey Vlad, are you suggesting that the boot parameter be made to
> support runtime? We can keep that for later as it may get complicated.
> Qais's boot parameter is designed only for boot time.
> 
No problem. Yes, i meant a runtime one. But as you stated there might
be hidden issues witch we are not aware of yet.

Thanks!

--
Uladzislau Rezki
  

On Fri, Dec 15, 2023 at 05:58:55PM +0100, Uladzislau Rezki wrote:
> Hello, Joel!
> 
> > [....]
> > > > > > +       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.
> > 
> > Hey Vlad, are you suggesting that the boot parameter be made to
> > support runtime? We can keep that for later as it may get complicated.
> > Qais's boot parameter is designed only for boot time.
> > 
> No problem. Yes, i meant a runtime one. But as you stated there might
> be hidden issues witch we are not aware of yet.

My current thought is that Qais's version currently in -rcu for
the merge window after next (v6.9) suits our current situation.
But if we are eventually able to support runtime changes to this new
rcutree.enable_rcu_lazy module parameter via simplification to the
rcu_nocb_try_bypass() function (or maybe a better analysis of it),
then at that point it would be good to allow this module parameter to
be changed via sysfs at runtime.

Does that make sense, or am I missing some aspect or use case?

							Thanx, Paul
  

On Fri, Dec 15, 2023 at 1:46 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Fri, Dec 15, 2023 at 05:58:55PM +0100, Uladzislau Rezki wrote:
> > Hello, Joel!
> >
> > > [....]
> > > > > > > +       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.
> > >
> > > Hey Vlad, are you suggesting that the boot parameter be made to
> > > support runtime? We can keep that for later as it may get complicated.
> > > Qais's boot parameter is designed only for boot time.
> > >
> > No problem. Yes, i meant a runtime one. But as you stated there might
> > be hidden issues witch we are not aware of yet.
>
> My current thought is that Qais's version currently in -rcu for
> the merge window after next (v6.9) suits our current situation.
> But if we are eventually able to support runtime changes to this new
> rcutree.enable_rcu_lazy module parameter via simplification to the
> rcu_nocb_try_bypass() function (or maybe a better analysis of it),
> then at that point it would be good to allow this module parameter to
> be changed via sysfs at runtime.

Yes, that's right.

> Does that make sense, or am I missing some aspect or use case?

No you are not missing anything.

Thanks.