linux-kernel - Re: [PATCH] cpufreq: schedutil: set sg_policy->next

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-next>] [day] [month] [year] [list]

Message-ID: <083a01d6ad19$fbdfbca0$f39f35e0$@gmail.com>
Date:   Wed, 28 Oct 2020 19:03:31 +0800
From:   "zhuguangqing83" <zhuguangqing83@...il.com>
To:     "'Viresh Kumar'" <viresh.kumar@...aro.org>
Cc:     <rjw@...ysocki.net>, <mingo@...hat.com>, <peterz@...radead.org>,
        <juri.lelli@...hat.com>, <vincent.guittot@...aro.org>,
        <dietmar.eggemann@....com>, <rostedt@...dmis.org>,
        <bsegall@...gle.com>, <mgorman@...e.de>, <bristot@...hat.com>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "'zhuguangqing'" <zhuguangqing@...omi.com>
Subject: Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq

> On 27-10-20, 19:54, zhuguangqing83@...il.com wrote:
> > From: zhuguangqing <zhuguangqing@...omi.com>
> >
> > In the following code path, next_freq is clamped between policy->min
> > and policy->max twice in functions cpufreq_driver_resolve_freq() and
> > cpufreq_driver_fast_switch(). For there is no update_lock in the code
> > path, policy->min and policy->max may be modified (one or more times),
> > so sg_policy->next_freq updated in function sugov_update_next_freq()
> > may be not the final cpufreq.
> 
> Understood until here, but not sure I followed everything after that.
> 
> > Next time when we use
> > "if (sg_policy->next_freq == next_freq)" to judge whether to update
> > next_freq, we may get a wrong result.
> >
> > -> sugov_update_single()
> >   -> get_next_freq()
> >     -> cpufreq_driver_resolve_freq()
> >   -> sugov_fast_switch()
> >     -> sugov_update_next_freq()
> >     -> cpufreq_driver_fast_switch()
> >
> > For example, at first sg_policy->next_freq is 1 GHz, but the final
> > cpufreq is 1.2 GHz because policy->min is modified to 1.2 GHz when
> > we reached cpufreq_driver_fast_switch(). Then next time, policy->min
> > is changed before we reached cpufreq_driver_resolve_freq() and (assume)
> > next_freq is 1 GHz, we find "if (sg_policy->next_freq == next_freq)" is
> > satisfied so we don't change the cpufreq. Actually we should change
> > the cpufreq to 1.0 GHz this time.
> 
> FWIW, whenever policy->min/max gets changed, sg_policy->limits_changed
> gets set to true by sugov_limits() and the next time schedutil
> callback gets called from the scheduler, we will fix the frequency.
> 
> And so there shouldn't be any issue here, unless I am missing
> something.

Thanks for your comments. Maybe my description was not clear before.

If I understand correctly, when policy->min/max get changed in the time
Window between get_next_freq() and sugov_fast_switch(), to be more
precise between cpufreq_driver_resolve_freq() and
cpufreq_driver_fast_switch(), the issue may happen.

For example, the first time schedutil callback gets called from the
scheduler, we reached get_next_freq() and calculate the next_freq,
suppose next_freq is 1.0 GHz, then sg_policy->next_freq is updated
to 1.0 GHz in sugov_update_next_freq(). If policy->min/max get
change right now, suppose policy->min is changed to 1.2 GHz, 
then the final next_freq is 1.2 GHz for there is another clamp
between policy->min and policy->max in cpufreq_driver_fast_switch().
Then sg_policy->next_freq(1.0 GHz) is not the final next_freq(1.2 GHz).

The second time schedutil callback gets called from the scheduler, there
are two issues:
(1) Suppose policy->min is still 1.2 GHz, we reached get_next_freq() and
calculate the next_freq, because sg_policy->limits_changed gets set to
true by sugov_limits() and there is a clamp between policy->min and
policy->max, so this time next_freq will be greater than or equal to 1.2
GHz, suppose next_freq is also 1.2 GHz. Now next_freq is 1.2 GHz and
sg_policy->next_freq is 1.0 GHz,  then we find
"if (sg_policy->next_freq == next_freq)" is not satisfied and we call
cpufreq driver to change the cpufreq to 1.2 GHz. Actually it's already
1.2 GHz, it's not necessary to change this time.

(2) Suppose policy->min was changed again to 1.0 GHz before, we reached
get_next_freq() and calculate the next_freq, suppose next_freq is also
1.0 GHz. Now next_freq is 1.0 GHz and sg_policy->next_freq is also 1.0 GHz,
then we find "if (sg_policy->next_freq == next_freq)" is satisfied and we
don't change the cpufreq. Actually we should change the cpufreq to 1.0 GHz
this time.

> 
> --
> viresh