Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <0d531690-1240-4356-a647-b4c71a56b31d@joelfernandes.org>
Date:   Sat, 9 Dec 2023 01:26:27 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] rcu: Provide a boot time parameter to control lazy RCU
Content-Language: en-US
To:     Qais Yousef <qyousef@layalina.io>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Zqiang <qiang.zhang1211@gmail.com>,
        Andrea Righi <andrea.righi@canonical.com>,
        John Stultz <jstultz@google.com>, linux-kernel@vger.kernel.org,
        rcu@vger.kernel.org
References: <20231203011252.233748-1-qyousef@layalina.io>
 <20231205162043.GA2558193@google.com>
 <20231207172032.kto27hfdxa4juq7b@airbuntu>
From:   Joel Fernandes <joel@joelfernandes.org>
In-Reply-To: <20231207172032.kto27hfdxa4juq7b@airbuntu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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