Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Rafael J. Wysocki" <rjw@rjwysocki.net>
To:     Peter Zijlstra <peterz@infradead.org>,
        Viresh Kumar <viresh.kumar@linaro.org>
Cc:     mingo@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org,
        linux-kernel@vger.kernel.org, Josh Poimboeuf <jpoimboe@redhat.com>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [RFC][PATCH] sched: Optimize cpufreq_update_util
Date:   Fri, 19 Mar 2021 15:35:43 +0100
Message-ID: <5452200.DvuYhMxLoT@kreacher>
In-Reply-To: <20210319073751.qz2ytpxl2ikrt2b7@vireshk-i7>
References: <20210318212826.GW4746@worktop.programming.kicks-ass.net> <20210319073751.qz2ytpxl2ikrt2b7@vireshk-i7>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Friday, March 19, 2021 8:37:51 AM CET Viresh Kumar wrote:
> On 18-03-21, 22:28, Peter Zijlstra wrote:
> > Also, is there a lock order comment in cpufreq somewhere?
> 
> I don't think so.
> 
> > I tried
> > following it, but eventually gave up and figured 'asking' lockdep was
> > far simpler.
> 
> This will get called from CPU's online/offline path at worst, nothing more.

I'm not sure if I understand you correctly, but for completeness the callback
is also set/unset on driver registration and governor switch.

> > +static void cpufreq_update_optimize(void)
> > +{
> > +	struct update_util_data *data;
> > +	cpu_util_update_f func = NULL, dfunc;
> > +	int cpu;
> > +
> > +	for_each_online_cpu(cpu) {
> > +		data = per_cpu(cpufreq_update_util_data, cpu);
> > +		dfunc = data ? READ_ONCE(data->func) : NULL;
> > +
> > +		if (dfunc) {
> > +			if (!func)
> > +				func = dfunc;
> > +			else if (func != dfunc)
> > +				return;
> > +		} else if (func)
> > +			return;
> > +	}
> 
> So there is nothing cpufreq specific IIRC that can help make this better, this
> is basically per policy.

Well, in some cases the driver knows that there will never be more that 1 CPU
per policy and so schedutil will never use the "shared" variant.

For instance, with intel_pstate all CPUs will always use the same callback.

> For example, on an ARM platform we have two cpufreq policies with one policy
> covering 4 CPUs, while the other one covering only 1 (maybe because we didn't
> add those CPUs in DT or something else), then also we will end up separate
> routines.
> 
> Or if we take all CPUs of a policy offline and then bring them up one by one, I
> think for the first CPU online event in that policy we will end up using the
> sugov_update_single_freq() variant for some time, until the time more CPUs come
> up.
> 
> So traversing the way you did this is probably something that will work properly
> in all corner cases.

Agreed.

It might be simplified in some cases, though, AFAICS.