linux-kernel - Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq

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]

Date:	Wed, 16 Mar 2016 14:12:39 +0100
From:	"Rafael J. Wysocki" <rafael@...nel.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	"Rafael J. Wysocki" <rafael@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Subject: Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

On Wed, Mar 16, 2016 at 5:47 AM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> On 15-03-16, 13:11, Rafael J. Wysocki wrote:
>> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>> > On 12-03-16, 03:05, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> >>
>> >> cpufreq_resume() attempts to resync the current frequency with
>> >> policy->cur for the first online CPU, but first it does that after
>> >> restarting governors for all active policies (which means that this
>> >> is racy with respect to whatever the governors do) and second it
>> >
>> > Why? Its doing the update withing policy->rwsem ..
>>
>> Which doesn't matter.
>>
>> dbs_work_handler() doesn't acquire policy->rwsem and may be executed
>> in parallel with this, for example.
>
> Right, so we need to fixup something here.
>
>> >> already is too late for that when cpufreq_resume() is called (that
>> >> happens after invoking ->resume callbacks for all devices in the
>> >> system).
>> >>
>> >> Also it doesn't make sense to do that for one CPU only in any case,
>> >> because the other CPUs in the system need not share the policy with
>> >> it and their policy->cur may be out of sync as well in principle.
>> >
>> > Its done just for the boot CPU, because that's the only CPU that goes to
>> > suspend. All other CPUs are disabled/enabled and so the policies are
>> > reinitialized for policy->cur as well.
>> >
>> > I think, its still important to get things in sync, as some bootloader may
>> > change the frequency to something else during resume.
>> >
>> > And our code may not be safe for the case, the current frequency of the CPU
>> > isn't part of the freq-table of the policy.
>>
>> Since we're already started the governor at this point (or called the
>> driver's ->resume), so the CPU is (or shortly will be) running at a
>> frequency that makes sense at this point.
>>
>> It might be running at a wrong one before, but not when this code is executed.
>
> Not necessarily.
>
> Consider Performance governor for example. Lets say policy->max is 1 GHz, so
> before suspend policy->cur will be 1 GHz. We suspended and resumed, and the
> bootloader changed the frequency to 500 MHz (but policy->cur remains the same at
> 1 GHz). Even after calling START for the governor, it will continue to run at
> 500 MHz.

No, it won't.  This might be applicable to other governors, but not to
"performance" (look at what it does on _START instead of just
guessing).

> So, your patch break things for sure.

I'm not actually sure it breaks anything.

Theoretically, it may, but practically?  Is there any system out there
where it makes any difference?