Messages in this thread | | | From | Joel Fernandes <> | Date | Tue, 12 Dec 2023 17:32:57 -0500 | Subject | Re: [PATCH v2] rcu: Provide a boot time parameter to control lazy RCU |
| |
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
| |