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@xxxxxxxxxxxxxxxxx> thanks, - Joel
