On Tue, Dec 12, 2023 at 7:05 AM Qais Yousef <qyousef@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> > > Thanks! > > -- > Qais Yousef
