linux-kernel - Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

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: <CANRm+CxFuTrr_-uqgwAdotR3_1+9eiCWuHDN0vbhmmKCKVbVxw@mail.gmail.com>
Date:	Tue, 31 May 2016 09:49:30 +0800
From:	Wanpeng Li <kernellwp@...il.com>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	Viresh Kumar <viresh.kumar@...aro.org>,
	Steve Muckle <steve.muckle@...aro.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Morten Rasmussen <morten.rasmussen@....com>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Juri Lelli <Juri.Lelli@....com>,
	Patrick Bellasi <patrick.bellasi@....com>,
	Michael Turquette <mturquette@...libre.com>
Subject: Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to
 driver frequency

2016-05-30 22:25 GMT+08:00 Rafael J. Wysocki <rafael@...nel.org>:
> On Mon, May 30, 2016 at 12:18 PM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>> On 29-05-16, 02:40, Rafael J. Wysocki wrote:
>>> I can't really parse the above question, so I'm not going to try to
>>> answer it. :-)
>>
>> Sorry about that :(
>>
>> IOW, I think that we should make this change into the sched-governor (I will
>> send a patch separately if you agree to this):
>
> I don't.
>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 14c4aa25cc45..5934b14aa21c 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -66,11 +66,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>>
>>         if (unlikely(sg_policy->need_freq_update)) {
>>                 sg_policy->need_freq_update = false;
>> -               /*
>> -                * This happens when limits change, so forget the previous
>> -                * next_freq value and force an update.
>> -                */
>> -               sg_policy->next_freq = UINT_MAX;
>>                 return true;
>>         }
>>
>> And here is my reasoning behind this.
>>
>> Can you show me any case, where the above code (as present in mainline
>> today) leads to a freq-change? I couldn't find any.
>>
>> Let me demonstrate.
>>
>> Following only talks about the fast-switch path, the other path is
>> same as well.
>>
>> Suppose this is the current range of frequencies supported by a
>> driver: 200, 400, 600, 800, 1000 (in MHz).
>>
>> And policy->cur = next_freq = 400 MHz.
>>
>> A.) Suppose that we change policy->min to 400 MHz from userspace.
>>     -> sugov_limits()
>>        This will find everything in order and simply set
>>        need_freq_update, without updating the frequency.
>>
>>     On next util-callback, we will forcefully return true from
>>     sugov_should_update_freq() and reach sugov_update_commit().
>>
>>     We calculate next_freq and that comes to 400 MHz again (that's the
>>     case we are trying to target with the above code).
>>
>>     With the current code, we will forcefully end up calling
>>     cpufreq_driver_fast_switch().
>>
>>     Because the new and current frequencies are same,
>>     cpufreq_driver->fast_switch() will simply return.
>>
>>     NOTE: I also think that cpufreq_driver_fast_switch() should have a
>>     check like (policy->cur == target_freq). I will add that too, in
>>     case you agree.
>>
>>     So, forcefully updating next_freq to UINT_MAX will end up wasting
>>     some cycles, but wouldn't do any useful stuff.
>
> It will, but there's no way to distinguish this case from B in the
> governor with the current min/max synchronization mechanism.  That is,
> it only knows that something has changed, but checking what exactly
> has changed would be racy.
>
>> B.) Suppose that we change policy->min to 600 MHz from userspace.
>>     -> sugov_limits()
>>        This will find that policy->cur is less than 600 and will set
>>        that to 600 MHz by calling __cpufreq_driver_target(). We will
>>        also set need_freq_update.
>>
>>        Note that next_freq and policy->cur are not in sync anymore and
>>        perhaps this is the most important case for the above code.
>
> It is.
>
> Moreover, please note that __cpufreq_driver_target() is only called in
> sugov_limits() when policy->fast_switch_enabled is unset.
>
>>     On next util-callback, we will forcefully return true from
>>     sugov_should_update_freq() and reach sugov_update_commit().
>>
>>     We calculate next_freq and lets say that comes to 400 MHz again
>>     (as that's the case we are trying to target with the above code).
>>
>>     With the current code, we will forcefully end up calling
>>     cpufreq_driver_fast_switch().
>>
>>     Because next_freq() is not part of the new range, we will clamp it
>>     and set it to 600 MHz eventually. Again, next and current
>>     frequencies are same, cpufreq_driver->fast_switch() will simply
>>     return.
>
> Not really (as per the above).
>
> And even in the !fast_switch_enabled case, if next_freq stays at 400
> MHz (which is different from policy->cur), it may lead to suboptimal
> decisions going forward (eg. if it goes to 600 MHz next time and the
> governor will think that the frequency has changed, although in fact
> it hasn't).

Does set next_freq = UNIT_MAX has same effect as next_freq stays at
400MHz since both means that frequency has changed?

Regards,
Wanpeng Li