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@xxxxxxxxxxxxxxxxx> Thanks! -- Qais Yousef
