Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 8 Apr 2022 10:49:08 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        rcu <rcu@vger.kernel.org>, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH RFC] rcu/nocb: Provide default all-CPUs mask for
 RCU_NOCB_CPU=y
Message-ID: <20220408174908.GK4285@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220407210734.2548973-1-joel@joelfernandes.org>
 <20220408142232.GA4285@paulmck-ThinkPad-P17-Gen-1>
 <CAEXW_YQWeqfcKdAKmCn4fFGyWXjOGd=29wvi6bL3k7s2bGkDJw@mail.gmail.com>
 <20220408155002.GF4285@paulmck-ThinkPad-P17-Gen-1>
 <CAEXW_YQDgSO2XkkVhN3RBBz3vwYdAtTuPz-xYYsAPnwEnbYZPA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEXW_YQDgSO2XkkVhN3RBBz3vwYdAtTuPz-xYYsAPnwEnbYZPA@mail.gmail.com>
Precedence: bulk

On Fri, Apr 08, 2022 at 01:20:02PM -0400, Joel Fernandes wrote:
> On Fri, Apr 8, 2022 at 11:50 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Fri, Apr 08, 2022 at 10:52:21AM -0400, Joel Fernandes wrote:
> > > On Fri, Apr 8, 2022 at 10:22 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > >
> > > > On Thu, Apr 07, 2022 at 09:07:33PM +0000, Joel Fernandes wrote:
> > > > > On systems with CONFIG_RCU_NOCB_CPU=y, there is no default mask provided
> > > > > which ends up not offloading any CPU. This patch removes yet another
> > > > > dependency from the bootloader having to know about RCU, about how many
> > > > > CPUs the system has, and about how to provide the mask. Basically, I
> > > > > think we should stop pretending that the user knows what they are doing :).
> > > > > In other words, if NO_CB_CPU is enabled, lets make use of it.
> > > > >
> > > > > My goal is to make RCU as zero-config as possible with sane defaults. If
> > > > > user wants to provide rcu_nocbs= or nohz_full= options, then those will
> > > > > take precedence and this patch will have no effect.
> > > > >
> > > > > I tested providing rcu_nocbs= option, ensuring that is preferred over this.
> > > >
> > > > Unless something has changed, this would change behavior relied upon
> > > > the enterprise distros.  Last I checked, they want to supply a single
> > > > binary, as evidenced by the recent CONFIG_PREEMPT_DYNAMIC Kconfig option,
> > > > and they also want the default to be non-offloaded.  That is, given a
> > > > kernel built with CONFIG_RCU_NOCB_CPU=y and without either a nohz_full
> > > > or a nocbs_cpu boot parameter, all of the CPUs must be non-offloaded.
> > >
> > > Just curious, do you have information (like data, experiment results)
> > > on why they want default non-offloaded? Or maybe they haven't tried
> > > the recent work done in NOCB code?
> >
> > I most definitely do.  When I first introduced callback offloading, I
> > made it completely replace softirq callback invocation.  There were some
> > important throughput-oriented workloads that got hit with significant
> > performance degradation due to this change.  Enterprise Java workloads
> > were the worst hit.
> >
> > Android does not run these workloads, and I am not aware of ChromeOS
> > running them, either.
> 
> Thanks a lot for mentioning this, I was not aware and will make note
> of it :-). I wonder if the scheduler had something to do with the
> degradation.

It is all too easy to blame the scheduler and all too easy to forget
that the scheduler has a hard job.  ;-)

And in this case, the scheduler was just doing what it was told.

With softirq, a common RCU code path sets a bit in a per-CPU variable.
With offloaded kthreads, that same code path does a much more expensive
wakeup operation.  In addition, in the common case, the softirq handler
runs on the back of an interrupt, and that interrupt handler has already
disrupted the caches, so additional disruption by the softirq handler
is not a big deal.  In contrast, the scheduler has to do the wakeup at
some point in time, and that point in time is very likely quite a bit
more disruptive.

> > > Another option I think is to make it enforce NOCB if NR_CPUS <= 32 if
> > > that makes sense.
> >
> > That would avoid hurting RHEL and SLES users, so this would be better
> > than making the change unconditionally.  But there are a lot of distros
> > out there.
> >
> > I have to ask...  Isn't there already a way of specifying a set of kernel
> > boot parameters that are required for ChromeOS?  If so, add rcu_nocbs=0-N
> > to that list and be happy.
> 
> Yes, that's doable.

Very good, thank you!

> > > > And is it really all -that- hard to specify an additional boot parameter
> > > > across ChromeOS devices?  Android seems to manage it.  ;-)
> > >
> > > That's not the hard part I think. The hard part is to make sure a
> > > future Linux user who is not an RCU expert does not forget to turn it
> > > on. ChromeOS is not the only OS that I've seen someone forget to do it
> > > ;-D. AFAIR, there were Android devices too in the past where I saw
> > > this forgotten. I don't think we should rely on the users doing the
> > > right thing (as much as possible).
> > >
> > > The single kernel binary point makes sense but in this case, I think
> > > the bigger question that I'd have is what is the default behavior and
> > > what do *most* users of RCU want. So we can keep sane defaults for the
> > > majority and reduce human errors related to configuration.
> >
> > If both the ChromeOS and Android guys need it, I could reinstate the
> > old RCU_NOCB_CPU_ALL Kconfig option.  This was removed due to complaints
> > about RCU Kconfig complexity, but I believe that Reviewed-by from ChromeOS
> > and Android movers and shakers would overcome lingering objections.
> >
> > Would that help?
> 
> Yes, I think I would love for such a change. I am planning to add a
> test to ChromeOS to check whether config options were correctly set
> up. So I can test for both the RCU_NOCB_CPU options.

Very good!

Do you love such a change enough to create the patch and to collect
convincing Reviewed-by tags?

							Thanx, Paul