Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: rjw@rjwysocki.net, catalin.marinas@arm.com, sudeep.holla@arm.com,
	will@kernel.org, linux@armlinux.org.uk,
	valentin.schneider@arm.com, mingo@redhat.com,
	peterz@infradead.org, dietmar.eggemann@arm.com,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Liviu Dudau <liviu.dudau@arm.com>
Subject: Re: [PATCH 4/8] cpufreq,vexpress-spc: fix Frequency Invariance (FI) for bL switching
Date: Wed, 1 Jul 2020 15:07:35 +0100
Message-ID: <20200701140735.GB32736@arm.com> (raw)
In-Reply-To: <20200701095414.2wjcnyhndgcedk2q@vireshk-i7>

On Wednesday 01 Jul 2020 at 16:16:19 (+0530), Viresh Kumar wrote:
> On 01-07-20, 10:07, Ionela Voinescu wrote:
> > In the majority of cases, the index argument to cpufreq's target_index()
> > is meant to identify the frequency that is requested from the hardware,
> > according to the frequency table: policy->freq_table[index].frequency.
> > 
> > After successfully requesting it from the hardware, this value, together
> > with the maximum hardware frequency (policy->cpuinfo.max_freq) are used
> > as arguments to arch_set_freq_scale(), in order to set the task scheduler
> > frequency scale factor. This is a normalized indication of a CPU's
> > current performance.
> > 
> > But for the vexpress-spc-cpufreq driver, when big.LITTLE switching [1]
> > is enabled, there are three issues with using the above information for
> > setting the FI scale factor:
> > 
> >  - cur_freq: policy->freq_table[index].frequency is not the frequency
> >    requested from the hardware. ve_spc_cpufreq_set_rate() will convert
> >    from this virtual frequency to an actual frequency, which is then
> >    requested from the hardware. For the A7 cluster, the virtual frequency
> >    is half the actual frequency. The use of the virtual policy->freq_table
> >    frequency results in an incorrect FI scale factor.
> > 
> >  - max_freq: policy->cpuinfo.max_freq does not correctly identify the
> >    maximum frequency of the physical cluster. This value identifies the
> >    maximum frequency achievable by the big-LITTLE pair, that is the
> >    maximum frequency of the big CPU. But when the LITTLE CPU in the group
> >    is used, the hardware maximum frquency passed to arch_set_freq_scale()
> >    is incorrect.
> > 
> >  - missing a scale factor update: when switching clusters, the driver
> >    recalculates the frequency of the old clock domain based on the
> >    requests of the remaining CPUs in the domain and asks for a clock
> >    change. But this does not result in an update in the scale factor.
> > 
> > Therefore, introduce a local function bLs_set_sched_freq_scale() that
> > helps call arch_set_freq_scale() with correct information for the
> > is_bL_switching_enabled() case, while maintaining the old, more
> > efficient, call site of arch_set_freq_scale() for when cluster
> > switching is disabled.
> > 
> > Also, because of these requirements in computing the scale factor, this
> > driver is the only one that maintains custom support for FI, which is
> > marked by the presence of the CPUFREQ_CUSTOM_SET_FREQ_SCALE flag.
> > 
> > [1] https://lwn.net/Articles/481055/
> > 
> > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > ---
> >  drivers/cpufreq/vexpress-spc-cpufreq.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> Is there anyone who cares for this driver and EAS ? I will just skip doing the
> FIE thing here and mark it skipped.
> 

That is a good question. The vexpress driver is still used for TC2, but
I don't know of any users of this bL switcher functionality that's part
of the driver. I think there were a few people wondering recently if
it's still used [1].

If we disconsider the bL switcher functionality, then the vexpress
driver itself gets in line with the other drivers supported by this
series. Therefore there would not be a flag set needed here. Also, to
maintain current functionality, we would not need to introduce a flag
at all.

But, the frequency invariance fix is also useful for schedutil to
better select a frequency based on invariant utilization. So if we
independently decide having a flag like the one introduced at 1/8 is
useful, I would recommend to consider this patch, as it does fix some
current functionality in the kernel (whether we can determine if it's
used much or not).

Therefore, IMO, if it's not used any more it should be removed, but if
it's kept it should be fixed.

[1] https://lore.kernel.org/linux-arm-kernel/20200624195811.435857-8-maz@kernel.org/


Thanks,
Ionela.


> -- 
> viresh

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-01  9:07 [PATCH 0/8] cpufreq: improve frequency invariance support Ionela Voinescu
2020-07-01  9:07 ` [PATCH 1/8] cpufreq: allow drivers to flag custom support for freq invariance Ionela Voinescu
2020-07-01 10:46   ` Viresh Kumar
2020-07-01 13:33     ` Ionela Voinescu
2020-07-01 16:05       ` Rafael J. Wysocki
2020-07-01 18:06         ` Ionela Voinescu
2020-07-02  2:58         ` Viresh Kumar
2020-07-02 11:44           ` Ionela Voinescu
2020-07-06 12:14             ` Dietmar Eggemann
2020-07-09  8:53               ` Ionela Voinescu
2020-07-09  9:09                 ` Viresh Kumar
2020-07-01  9:07 ` [PATCH 2/8] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
2020-07-01 10:46   ` Viresh Kumar
2020-07-01 15:27     ` Ionela Voinescu
2020-07-01 15:51       ` Rafael J. Wysocki
2020-07-02  3:01         ` Viresh Kumar
2020-07-02 11:45         ` Ionela Voinescu
2020-07-01  9:07 ` [PATCH 3/8] cpufreq,drivers: remove setting of frequency scale factor Ionela Voinescu
2020-07-01  9:07 ` [PATCH 4/8] cpufreq,vexpress-spc: fix Frequency Invariance (FI) for bL switching Ionela Voinescu
2020-07-01 10:46   ` Viresh Kumar
2020-07-01 14:07     ` Ionela Voinescu [this message]
2020-07-02  3:05       ` Viresh Kumar
2020-07-02 11:41         ` Ionela Voinescu
2020-07-02 11:46           ` Viresh Kumar
2020-07-01  9:07 ` [PATCH 5/8] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
2020-07-01 10:46   ` Viresh Kumar
2020-07-01  9:07 ` [PATCH 6/8] arch_topology,cpufreq,sched/core: constify arch_* cpumasks Ionela Voinescu
2020-07-01  9:07 ` [PATCH 7/8] arch_topology,arm64: define arch_scale_freq_invariant() Ionela Voinescu
2020-07-01  9:07 ` [PATCH 8/8] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701140735.GB32736@arm.com \
    --to=ionela.voinescu@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=liviu.dudau@arm.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sudeep.holla@arm.com \
    --cc=valentin.schneider@arm.com \
    --cc=viresh.kumar@linaro.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git