linux-kernel - Re: [PATCH] cpufreq: conservative: Fix requested

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2671493.AgiGORq3CZ@aspire.rjw.lan>
Date:   Thu, 11 Oct 2018 23:06:58 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Waldemar Rymarkiewicz <waldemar.rymarkiewicz@...il.com>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@...el.com>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Bartholomae, Thomas" <t.bartholomae@...el.com>
Subject: Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > <waldemar.rymarkiewicz@...il.com> wrote:
> > >
> > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@...el.com>
> > >
> > > The governor updates dbs_info->requested_freq only after increasing or
> > > decreasing frequency. There is, however, an use case when this is not
> > > sufficient.
> > >
> > > Imagine, external module constraining cpufreq policy in a way that policy->max
> >
> > Is the "external module" here a utility or a demon running in user space?
> 
> No, this is a driver that communicates with a firmware and makes sure
> CPU is running at the highest rate in specific time.
> It uses verify_within_limits  and update_policy, a standard way to
> constraint cpufreq policy limits.
> 
> > > @@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
> > >                         requested_freq = policy->min;
> > >
> > >                 __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
> > > -               dbs_info->requested_freq = requested_freq;
> > >         }
> > >
> > >   out:
> > > +       dbs_info->requested_freq = requested_freq;
> >
> > This will have a side effect when requested_freq is updated before the
> > thresholds checks due to the policy_dbs->idle_periods < UINT_MAX
> > check.
> >
> > Shouldn't that be avoided?
> 
> I would say we should.
> 
> A hardware design I use is running 4.9 kernel where the check does not
> exist yet, so there is not a problem.
> Anyway, the check policy_dbs->idle_periods < UINT_MAX  can change
> requested_freq  either to requested_freq = policy->min or
> requested_freq -= freq_steps;. The first case will not change anything
> for us as policy->max=min=cur. The second, however, will force to
> update freq which is definitely not expected when limits are set to
> min=max. Simply it will not go out  here:
> 
> if (load < cs_tuners->down_threshold) {
>       if (requested_freq == policy->min)
>            goto out;   <---
> ...
> }
> 
> Am I right here? If so, shouldn't we check explicitly
> 
> /*
> * If requested_freq is out of range, it is likely that the limits
> * changed in the meantime, so fall back to current frequency in that
> * case.
> */
> if (requested_freq > policy->max || requested_freq < policy->min)
>        requested_freq = policy->cur;
> 
> +/*
> +* If the the new limits min,max are equal, there is no point to process further
> +*/
> +
> +if (requested_freq == policy->max  &&  requested_freq == policy->min)
> +     goto out;

If my understanding of the problem is correct, it would be better to simply
update dbs_info->requested_freq along with requested_freq when that is found
to be out of range.  IOW, something like the appended patch (untested).

Wouldn't that address the problem at hand?

---
 drivers/cpufreq/cpufreq_conservative.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -80,8 +80,10 @@ static unsigned int cs_dbs_update(struct
 	 * changed in the meantime, so fall back to current frequency in that
 	 * case.
 	 */
-	if (requested_freq > policy->max || requested_freq < policy->min)
+	if (requested_freq > policy->max || requested_freq < policy->min) {
 		requested_freq = policy->cur;
+		dbs_info->requested_freq = requested_freq;
+	}
 
 	freq_step = get_freq_step(cs_tuners, policy);